From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754190AbdDJRs1 (ORCPT ); Mon, 10 Apr 2017 13:48:27 -0400 Received: from mx1.redhat.com ([209.132.183.28]:45836 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753603AbdDJRsY (ORCPT ); Mon, 10 Apr 2017 13:48:24 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 3E8B3C059727 Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx08.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=tcamuso@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com 3E8B3C059727 Subject: Re: [PATCH] ipmi_si: use smi_num for init_name To: minyard@acm.org References: <1491846217-384-1-git-send-email-minyard@acm.org> Cc: openipmi-developer@lists.sourceforge.net, linux-kernel@vger.kernel.org, Corey Minyard From: Tony Camuso Message-ID: <516358b2-28f9-c036-65ab-aa05fd5ed6b7@redhat.com> Date: Mon, 10 Apr 2017 13:48:23 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: <1491846217-384-1-git-send-email-minyard@acm.org> Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.32]); Mon, 10 Apr 2017 17:48:24 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 04/10/2017 01:43 PM, minyard@acm.org wrote: > From: Tony Camuso > > Commit 1abf71e moved the creation of new_smi->dev to earlier in the init > sequence in order to provide infrastructure for log printing. > > However, the init_name was created with a hard-coded value of zero. This > presents a problem in systems with more than one interface, producing a > call trace in dmesg. > > To correct the problem, simply use smi_num instead of the hard-coded > value of zero. > > Tested on a lenovo x3950. > > Signed-off-by: Tony Camuso > > There was actually a more general problem, the platform device wasn't > being set correctly, either, and there was a possible (though extremely > unlikely) race on smi_num. Add locks to clean up the race and use the > proper value for the platform device, too. Ouch! I thought smi_num was already locked by smi_infos. Now I see that it wasn't. Thanks! Regards, tony > > Tested on qemu in various configurations. > > Signed-off-by: Corey Minyard > --- > > I reworked this a bit, I think I've covered all the problems with > smi_num. Tony, does this work for you? > > Thanks > > drivers/char/ipmi/ipmi_si_intf.c | 19 ++++++++++++++++--- > 1 file changed, 16 insertions(+), 3 deletions(-) > > diff --git a/drivers/char/ipmi/ipmi_si_intf.c b/drivers/char/ipmi/ipmi_si_intf.c > index 2a7c425..b2b618f 100644 > --- a/drivers/char/ipmi/ipmi_si_intf.c > +++ b/drivers/char/ipmi/ipmi_si_intf.c > @@ -1954,7 +1954,9 @@ static int hotmod_handler(const char *val, struct kernel_param *kp) > kfree(info); > goto out; > } > + mutex_lock(&smi_infos_lock); > rv = try_smi_init(info); > + mutex_unlock(&smi_infos_lock); > if (rv) { > cleanup_one_si(info); > goto out; > @@ -2042,8 +2044,10 @@ static int hardcode_find_bmc(void) > info->slave_addr = slave_addrs[i]; > > if (!add_smi(info)) { > + mutex_lock(&smi_infos_lock); > if (try_smi_init(info)) > cleanup_one_si(info); > + mutex_unlock(&smi_infos_lock); > ret = 0; > } else { > kfree(info); > @@ -3492,6 +3496,11 @@ static int add_smi(struct smi_info *new_smi) > return rv; > } > > +/* > + * Try to start up an interface. Must be called with smi_infos_lock > + * held, primarily to keep smi_num consistent, we only one to do these > + * one at a time. > + */ > static int try_smi_init(struct smi_info *new_smi) > { > int rv = 0; > @@ -3524,9 +3533,12 @@ static int try_smi_init(struct smi_info *new_smi) > goto out_err; > } > > + new_smi->intf_num = smi_num; > + > /* Do this early so it's available for logs. */ > if (!new_smi->dev) { > - init_name = kasprintf(GFP_KERNEL, "ipmi_si.%d", 0); > + init_name = kasprintf(GFP_KERNEL, "ipmi_si.%d", > + new_smi->intf_num); > > /* > * If we don't already have a device from something > @@ -3593,8 +3605,6 @@ static int try_smi_init(struct smi_info *new_smi) > > new_smi->interrupt_disabled = true; > atomic_set(&new_smi->need_watch, 0); > - new_smi->intf_num = smi_num; > - smi_num++; > > rv = try_enable_event_buffer(new_smi); > if (rv == 0) > @@ -3661,6 +3671,9 @@ static int try_smi_init(struct smi_info *new_smi) > goto out_err_stop_timer; > } > > + /* Don't increment till we know we have succeeded. */ > + smi_num++; > + > dev_info(new_smi->dev, "IPMI %s interface initialized\n", > si_to_str[new_smi->si_type]); > >