From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1031423AbbJ3OKk (ORCPT ); Fri, 30 Oct 2015 10:10:40 -0400 Received: from mout.kundenserver.de ([212.227.126.131]:52612 "EHLO mout.kundenserver.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1030592AbbJ3OKh (ORCPT ); Fri, 30 Oct 2015 10:10:37 -0400 From: Arnd Bergmann To: John Garry Cc: JBottomley@odin.com, robh+dt@kernel.org, pawel.moll@arm.com, mark.rutland@arm.com, ijc+devicetree@hellion.org.uk, galak@codeaurora.org, linux-scsi@vger.kernel.org, linux-kernel@vger.kernel.org, devicetree@vger.kernel.org, linuxarm@huawei.com, john.garry2@mail.dcu.ie, hare@suse.de, xuwei5@hisilicon.com, zhangfei.gao@linaro.org Subject: Re: [PATCH v2 25/32] scsi: hisi_sas: add abnormal irq handler Date: Fri, 30 Oct 2015 15:10:24 +0100 Message-ID: <3709094.hsDE2JcmxW@wuerfel> User-Agent: KMail/4.11.5 (Linux/3.16.0-10-generic; KDE/4.11.5; x86_64; ; ) In-Reply-To: <1445868903-183817-26-git-send-email-john.garry@huawei.com> References: <1445868903-183817-1-git-send-email-john.garry@huawei.com> <1445868903-183817-26-git-send-email-john.garry@huawei.com> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" X-Provags-ID: V03:K0:Fi8Q6QPiI/M37dEcUmTONuWhb6pmJ5gJ1aJKQUQee0E2lk9thrk ZwMrqd4Ic1chECB7Ibv5oCO+jYGHNU8RiZfZnyDzlP2AIajbDwqsiytMtRc5GORIIfnnZ0p CjVHx8GyFa0TvwvGoNhdulj8S5pdnzgh0qtPd4euBO/HhWjwLYfdAl2GFs9cmIpGyviYMuw 7XLyM1DlJzwBSxKzxJU9Q== X-UI-Out-Filterresults: notjunk:1;V01:K0:ELBkB3hxyk4=:uzkeJyjVW02+LUWUNEimxF Je9OTfmbvoQDcKM/zPUh10HcVsdMbljHzmxL8s3gMDb4ZTKJtCp45B3DRorA1JvuMz8CXdG97 /52dMKc+4e6PgW8mFA7eZXR7MxgrcZpx6DOtdxSi3rSqYkt0JQbqcR1qpTjHRuQuSw8apAHFD hJjfjwlo9e0U0MeyACYwS0L+VCKNEWB/tmy3bpWpNDjYlDQWSUgcIXKgLMIz86S+uF5d96Td1 DsvltXsfPEA3myT0JOx/2oYjMonQQ9yfv6h7GgVwJNtypX42B9UWQtnKv6lWSooU4/2Nst6Ak VqCbVrOqiDzz4iXIthGYcArQ7ZQT0bGQoyh5mDnF1yTI84mLJEmJYCyurNqCYJwZpr8iXIjjC q6Y1bxC61PADSwWaIPx08/EDv9uZeBs/aypcExqxMODwln0h489IhagFpmbxjRIuxiVqjnRdO Ivx4idedReZKMQkJ7C0bYw35CgcsIMIoxLwRRgdNO9nc2CT4AVcy6vhGECa3rAATI8v6txpvb YpaRwCZTLoNw+wiBwE0rAz7MUrHcfaqrxVj2Z+et1zgFZ7BdJ7ukFHmTV0UKvRRMxqlfO20FW Uk5VgZFU4pu4NaYk3yq03iLIR+bnOsvSPUzb4PcpDBqhRhs9EtIdWUVn0Z3LeyXIL4cI6NsRT KPMS0kuH8o0vPXReFbgsDWt+pIDVFS3b9m7KpSw/63A156RsOlI2jhyESyDPMSiEMgkofCUnQ uskWl2wVIDfTwdrY Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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. > + if (!sas_port) > + return; > + > + while (sas_ha->sas_phy[i]) { Using a for() loop would avoid the initialization here. > + 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. > + 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. > + 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. > + 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. Arnd