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

[Arnd Bergmann]

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.

> +	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

--
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/