Re: [PATCH #upstream-fixes] ata_piix: fix locking around SIDPR access

[David Milburn <dmilburn@redhat.com>]

Tejun Heo wrote:
> SIDPR window registers are shared across ports and as each access is
> done in two steps, accesses to different ports under EH may race.
> This primarily is caused by incorrect host locking in EH context and
> should be fixed by defining locking requirements for each EH operation
> which can be used during EH and enforcing them but for now work around
> the problem by adding a dedicated SIDPR lock and grabbing it for each
> SIDPR access.

Hello Tejun,

I have confirmation of a successful 1000 reboot test after applying
this patch. Previously, ata_piix system failed to boot frequently.

Thanks,
David

> 
> Signed-off-by: Tejun Heo <tj@kernel.org>
> Reported-by: Mark Knecht <markknecht@gmail.com>
> Reported-by: Paul Check <paul@thechecks.ca>
> Cc: stable@kernel.org
> ---
>  drivers/ata/ata_piix.c |    8 ++++++++
>  1 file changed, 8 insertions(+)
> 
> diff --git a/drivers/ata/ata_piix.c b/drivers/ata/ata_piix.c
> index 7409f98..3971bc0 100644
> --- a/drivers/ata/ata_piix.c
> +++ b/drivers/ata/ata_piix.c
> @@ -158,6 +158,7 @@ struct piix_map_db {
>  struct piix_host_priv {
>  	const int *map;
>  	u32 saved_iocfg;
> +	spinlock_t sidpr_lock;	/* FIXME: remove once locking in EH is fixed */
>  	void __iomem *sidpr;
>  };
> 
> @@ -951,12 +952,15 @@ static int piix_sidpr_scr_read(struct ata_link *link,
>  			       unsigned int reg, u32 *val)
>  {
>  	struct piix_host_priv *hpriv = link->ap->host->private_data;
> +	unsigned long flags;
> 
>  	if (reg >= ARRAY_SIZE(piix_sidx_map))
>  		return -EINVAL;
> 
> +	spin_lock_irqsave(&hpriv->sidpr_lock, flags);
>  	piix_sidpr_sel(link, reg);
>  	*val = ioread32(hpriv->sidpr + PIIX_SIDPR_DATA);
> +	spin_unlock_irqrestore(&hpriv->sidpr_lock, flags);
>  	return 0;
>  }
> 
> @@ -964,12 +968,15 @@ static int piix_sidpr_scr_write(struct ata_link *link,
>  				unsigned int reg, u32 val)
>  {
>  	struct piix_host_priv *hpriv = link->ap->host->private_data;
> +	unsigned long flags;
> 
>  	if (reg >= ARRAY_SIZE(piix_sidx_map))
>  		return -EINVAL;
> 
> +	spin_lock_irqsave(&hpriv->sidpr_lock, flags);
>  	piix_sidpr_sel(link, reg);
>  	iowrite32(val, hpriv->sidpr + PIIX_SIDPR_DATA);
> +	spin_unlock_irqrestore(&hpriv->sidpr_lock, flags);
>  	return 0;
>  }
> 
> @@ -1566,6 +1573,7 @@ static int __devinit piix_init_one(struct pci_dev *pdev,
>  	hpriv = devm_kzalloc(dev, sizeof(*hpriv), GFP_KERNEL);
>  	if (!hpriv)
>  		return -ENOMEM;
> +	spin_lock_init(&hpriv->sidpr_lock);
> 
>  	/* Save IOCFG, this will be used for cable detection, quirk
>  	 * detection and restoration on detach.  This is necessary
> --
> To unsubscribe from this list: send the line "unsubscribe linux-ide" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html