linux-next: hdparm -B <value> broken

linux-next: hdparm -B <value> broken

[Sabrina Dubroca]

Hello,

Since next-20140823:

# hdparm -B 200 /dev/sda

/dev/sda:
 setting Advanced Power Management level to 0xc8 (200)
 HDIO_DRIVE_CMD failed: Bad address
 APM_level      = 128


It looks like before commit 2cada584b200 ("block: cleanup error
handling in sg_io"), we had ret = 0 before entering the last big if
block of sg_io, and now ret = -EFAULT.

The following patch seems to fix the problem:


diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 5dd477bfb4bc..9b8eaeca6a79 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -330,6 +330,7 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk,
 	if (blk_fill_sghdr_rq(q, rq, hdr, mode))
 		goto out_free_cdb;
 
+	ret = 0;
 	if (hdr->iovec_count) {
 		size_t iov_data_len;
 		struct iovec *iov = NULL;



Thanks,

-- 
Sabrina

Re: linux-next: hdparm -B <value> broken

[Christoph Hellwig]

On Tue, Aug 26, 2014 at 03:31:59PM +0200, Sabrina Dubroca wrote:
> The following patch seems to fix the problem:

Thanks for the fix!  Can you turn this into a patch with a proper subject and a
signoff?

[PATCH] block: fix error handling in sg_io

[Sabrina Dubroca]

Before commit 2cada584b200 ("block: cleanup error handling in sg_io"),
we had ret = 0 before entering the last big if block of sg_io.

Since 2cada584b200, ret = -EFAULT, which breaks hdparm:

/dev/sda:
 setting Advanced Power Management level to 0xc8 (200)
 HDIO_DRIVE_CMD failed: Bad address
 APM_level      = 128

Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
Fixes: 2cada584b200 ("block: cleanup error handling in sg_io")
---
 block/scsi_ioctl.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 5dd477bfb4bc..9b8eaeca6a79 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -330,6 +330,7 @@ static int sg_io(struct request_queue *q, struct gendisk *bd_disk,
 	if (blk_fill_sghdr_rq(q, rq, hdr, mode))
 		goto out_free_cdb;
 
+	ret = 0;
 	if (hdr->iovec_count) {
 		size_t iov_data_len;
 		struct iovec *iov = NULL;
-- 
2.1.0

Re: [PATCH] block: fix error handling in sg_io

[Jens Axboe]

On 08/26/2014 08:14 AM, Sabrina Dubroca wrote:
> Before commit 2cada584b200 ("block: cleanup error handling in sg_io"),
> we had ret = 0 before entering the last big if block of sg_io.
> 
> Since 2cada584b200, ret = -EFAULT, which breaks hdparm:
> 
> /dev/sda:
>  setting Advanced Power Management level to 0xc8 (200)
>  HDIO_DRIVE_CMD failed: Bad address
>  APM_level      = 128
> 
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> Fixes: 2cada584b200 ("block: cleanup error handling in sg_io")

Thanks, good catch. Someone else reported a regression with identical
symptoms to me yesterday, but did not follow up, so I'm glad you both
found and fixed it.


-- 
Jens Axboe

Re: [PATCH] block: fix error handling in sg_io

[Christoph Hellwig]

On Tue, Aug 26, 2014 at 04:14:02PM +0200, Sabrina Dubroca wrote:
> Before commit 2cada584b200 ("block: cleanup error handling in sg_io"),
> we had ret = 0 before entering the last big if block of sg_io.
> 
> Since 2cada584b200, ret = -EFAULT, which breaks hdparm:
> 
> /dev/sda:
>  setting Advanced Power Management level to 0xc8 (200)
>  HDIO_DRIVE_CMD failed: Bad address
>  APM_level      = 128
> 
> Signed-off-by: Sabrina Dubroca <sd@queasysnail.net>
> Fixes: 2cada584b200 ("block: cleanup error handling in sg_io")

Looks good,

Reviewed-by: Christoph Hellwig <hch@lst.de>