re: libata: Implement disk shock protection support

re: libata: Implement disk shock protection support

[Dan Carpenter]

Hello Elias Oltmanns,

The patch 45fabbb77bd9: "libata: Implement disk shock protection
support" from Sep 21, 2008, leads to the following Smatch
warning:

  drivers/ata/libata-scsi.c:206 ata_scsi_park_show()
  warn: inconsistent returns 'irqsave:flags'.
	  Locked on:   line 206
	  Unlocked on: line 206

drivers/ata/libata-scsi.c
   170  static ssize_t ata_scsi_park_show(struct device *device,
   171                                    struct device_attribute *attr, char *buf)
   172  {
   173          struct scsi_device *sdev = to_scsi_device(device);
   174          struct ata_port *ap;
   175          struct ata_link *link;
   176          struct ata_device *dev;
   177          unsigned long flags, now;
   178          unsigned int uninitialized_var(msecs);
   179          int rc = 0;
   180  
   181          ap = ata_shost_to_port(sdev->host);
   182  
   183          spin_lock_irqsave(ap->lock, flags);
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   184          dev = ata_scsi_find_dev(ap, sdev);
   185          if (!dev) {
   186                  rc = -ENODEV;
   187                  goto unlock;
   188          }
   189          if (dev->flags & ATA_DFLAG_NO_UNLOAD) {
   190                  rc = -EOPNOTSUPP;
   191                  goto unlock;
   192          }
   193  
   194          link = dev->link;
   195          now = jiffies;
   196          if (ap->pflags & ATA_PFLAG_EH_IN_PROGRESS &&
   197              link->eh_context.unloaded_mask & (1 << dev->devno) &&
   198              time_after(dev->unpark_deadline, now))
   199                  msecs = jiffies_to_msecs(dev->unpark_deadline - now);
   200          else
   201                  msecs = 0;
   202  
   203  unlock:
   204          spin_unlock_irq(ap->lock);
                ^^^^^^^^^^^^^^^^^^^^^^^^^

This should almost certainly be spin_unlock_irqrestore().

   205  
   206          return rc ? rc : snprintf(buf, 20, "%u\n", msecs);
   207  }


regards,
dan carpenter

Re: libata: Implement disk shock protection support

[Elias Oltmanns]

Hi Dan,

On 17 February 2016 at 20:24 CET, Dan Carpenter wrote:
> Hello Elias Oltmanns,
> The patch 45fabbb77bd9: "libata: Implement disk shock protection
> support" from Sep 21, 2008, leads to the following Smatch
> warning:
> 
>   drivers/ata/libata-scsi.c:206 ata_scsi_park_show()
>   warn: inconsistent returns 'irqsave:flags'.
> 	  Locked on:   line 206
> 	  Unlocked on: line 206
> 
> drivers/ata/libata-scsi.c
>    170  static ssize_t ata_scsi_park_show(struct device *device,
>    171                                    struct device_attribute *attr, char *buf)
>    172  {
[...]
>    183          spin_lock_irqsave(ap->lock, flags);
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
[...]
>    204          spin_unlock_irq(ap->lock);
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^
> 
> This should almost certainly be spin_unlock_irqrestore().

since this is an accessor function for a sysfs attribute (see:
DEVICE_ATTR(unload_heads, S_IRUGO | S_IWUSR,
	    ata_scsi_park_show, ata_scsi_park_store);
at line 269), it will only ever be called from process context, in my
estimation. So, I suggest using spin_lock_irq() rather than the
_irqsave() variant. The same goes for the ata_scsi_park_store()
function.

Thanks for pointing out the inconsistency. Even though the patch itself
is quite straight forward, I won't have time to even compile test it
before the weekend.

Regards,

Elias

[PATCH libata/for-4.6] libata: fix unbalanced spin_lock_irqsave/spin_unlock_irq() in ata_scsi_park_show()

[Tejun Heo]

>From 3948b6f2b7677165324afe52c3bd0088ca7c776c Mon Sep 17 00:00:00 2001
From: Tejun Heo <tj@kernel.org>
Date: Thu, 18 Feb 2016 11:50:37 -0500

ata_scsi_park_show() was pairing spin_lock_irqsave() with
spin_unlock_irq().  As the function is always called with irq enabled,
it didn't actually break anything.  Use spin_lock_irq() instead.

Signed-off-by: Tejun Heo <tj@kernel.org>
Reported-by: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Elias Oltmanns <eo@nebensachen.de>
---
Applied to libata/for-4.6.

Thanks.

 drivers/ata/libata-scsi.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index e417e1a..567859c 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -174,13 +174,13 @@ static ssize_t ata_scsi_park_show(struct device *device,
 	struct ata_port *ap;
 	struct ata_link *link;
 	struct ata_device *dev;
-	unsigned long flags, now;
+	unsigned long now;
 	unsigned int uninitialized_var(msecs);
 	int rc = 0;
 
 	ap = ata_shost_to_port(sdev->host);
 
-	spin_lock_irqsave(ap->lock, flags);
+	spin_lock_irq(ap->lock);
 	dev = ata_scsi_find_dev(ap, sdev);
 	if (!dev) {
 		rc = -ENODEV;
-- 
2.5.0

i

Re: [PATCH libata/for-4.6] libata: fix unbalanced spin_lock_irqsave/spin_unlock_irq() in ata_scsi_park_show()

[Elias Oltmanns]

On 18 February 2016 at 17:54 CET, Tejun Heo wrote:
>> From 3948b6f2b7677165324afe52c3bd0088ca7c776c Mon Sep 17 00:00:00 2001
> From: Tejun Heo <tj@kernel.org>
> Date: Thu, 18 Feb 2016 11:50:37 -0500
> 
> ata_scsi_park_show() was pairing spin_lock_irqsave() with
> spin_unlock_irq().  As the function is always called with irq enabled,
> it didn't actually break anything.  Use spin_lock_irq() instead.

Thanks for taking care of this. As I stated earlier, I'd suggest exactly
the same change for ata_scsc_park_store() too, though.

Regards,

Elias

Re: [PATCH libata/for-4.6] libata: fix unbalanced spin_lock_irqsave/spin_unlock_irq() in ata_scsi_park_show()

[Tejun Heo]

On Fri, Feb 19, 2016 at 07:49:57AM +0100, Elias Oltmanns wrote:
> On 18 February 2016 at 17:54 CET, Tejun Heo wrote:
> >> From 3948b6f2b7677165324afe52c3bd0088ca7c776c Mon Sep 17 00:00:00 2001
> > From: Tejun Heo <tj@kernel.org>
> > Date: Thu, 18 Feb 2016 11:50:37 -0500
> > 
> > ata_scsi_park_show() was pairing spin_lock_irqsave() with
> > spin_unlock_irq().  As the function is always called with irq enabled,
> > it didn't actually break anything.  Use spin_lock_irq() instead.
> 
> Thanks for taking care of this. As I stated earlier, I'd suggest exactly
> the same change for ata_scsc_park_store() too, though.

Send a patch?

-- 
tejun