Re: [PATCH v2 25/32] scsi: hisi_sas: add abnormal irq handler

[John Garry]

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 <john.garry@xxxxxxxxxx>
>
> 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@xxxxxxxxxxxxxxx
> 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

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/