From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1759238AbbJ3Q7i (ORCPT ); Fri, 30 Oct 2015 12:59:38 -0400 Received: from szxga02-in.huawei.com ([119.145.14.65]:55514 "EHLO szxga02-in.huawei.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758350AbbJ3Q7g (ORCPT ); Fri, 30 Oct 2015 12:59:36 -0400 Subject: Re: [PATCH v2 25/32] scsi: hisi_sas: add abnormal irq handler To: Arnd Bergmann References: <1445868903-183817-1-git-send-email-john.garry@huawei.com> <1445868903-183817-26-git-send-email-john.garry@huawei.com> <3709094.hsDE2JcmxW@wuerfel> CC: , , , , , , , , , , , , , From: John Garry Message-ID: <5633A19A.1010900@huawei.com> Date: Fri, 30 Oct 2015 16:58:02 +0000 User-Agent: Mozilla/5.0 (Windows NT 6.1; rv:38.0) Gecko/20100101 Thunderbird/38.2.0 MIME-Version: 1.0 In-Reply-To: <3709094.hsDE2JcmxW@wuerfel> Content-Type: text/plain; charset="windows-1252"; format=flowed Content-Transfer-Encoding: 7bit X-Originating-IP: [10.202.137.251] X-CFilter-Loop: Reflected Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 30/10/2015 14:10, Arnd Bergmann wrote: > On Monday 26 October 2015 22:14:56 John Garry wrote: >> Add abnormal irq handler. This handler is concerned with >> phy down event. >> Also add port formed and port deformed handlers. >> >> Signed-off-by: John Garry > > I noticed a couple more coding style issues in this patch than elsewhere, so here > is a slightly more detailed review. > >> +static void hisi_sas_port_notify_formed(struct asd_sas_phy *sas_phy, int lock) >> +{ >> + struct sas_ha_struct *sas_ha = sas_phy->ha; >> + struct hisi_hba *hisi_hba = NULL; >> + int i = 0; >> + struct hisi_sas_phy *phy = sas_phy->lldd_phy; >> + struct asd_sas_port *sas_port = sas_phy->port; >> + struct hisi_sas_port *port; >> + unsigned long flags = 0; > > Here and in general, please avoid initializing local variables > to zero, as that prevents gcc from warning about uses that > come before the real initialization. > > The flags that get passed into spin_lock_irqsave() are architecture > specific, so you cannot rely on '0' to have a particular meaning. > Noted. Will change. >> + if (!sas_port) >> + return; >> + >> + while (sas_ha->sas_phy[i]) { > > Using a for() loop would avoid the initialization here. > Yes. >> + if (sas_ha->sas_phy[i] == sas_phy) { >> + hisi_hba = (struct hisi_hba *)sas_ha->lldd_ha; > > lldd_ha is a void pointer, so you don't need a cast. > Noted. >> + port = &hisi_hba->port[i]; >> + break; >> + } >> + i++; >> + } > > The loop is really odd, as you apparently only try to find the > array index to a pointer you already have. Is there no space for > driver-specific data in 'asd_sas_phy'? If there is, just point to > per-phy structure that you define yourself and put the index into > that structure. I believe you already have a struct like that. > This code is a remnant from when we used to have the driver designed in such a way that we had a single Scsi Host and which had multiple cores (and hisi_hba's). Will address. >> + if (hisi_hba == NULL) { > > When checking a pointer for validity, do not compare against > NULL, but write this as 'if (!hisi_hba)', which is the more > normal coding style. OK. > >> + pr_err("%s: could not find hba\n", __func__); >> + return; >> + } > > Better use dev_err() to print the device name, but remove the __func__ > argument. Again, when you have the per-phy structure, put a pointer > to the device in there. > >> + >> + if (lock) >> + spin_lock_irqsave(&hisi_hba->lock, flags); >> + port->port_attached = 1; >> + port->id = phy->port_id; >> + phy->port = port; >> + sas_port->lldd_port = port; >> + >> + if (lock) >> + spin_unlock_irqrestore(&hisi_hba->lock, flags); >> +} > > This breaks some checking tools that try to validate the uses of > locks. Better wrap the function in another one depending on the > caller. When using spinlocks in general, it's also better to replace > the "I have no clue where I'm called from" spin_lock_irqsave() > call with either spin_lock() or spin_lock_irq() if at all possible. > Ok, I can remove the lock check. I know that the function can be called from irq context in my irq handler and also from multiple functions in libsas (whose context I am unsure of). > Arnd > > -- > To unsubscribe from this list: send the line "unsubscribe linux-scsi" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > > . > Thanks for the detailed review. I'll try and address your comments for other patches. john