RE: [PATCH RFC 0/3] Add ufs provisioning support in driver

[sayali]

Hi Kyuho Choi,

Thank you for your comments.

Yes, there can be multiple use-cases where we want to re-provision a ufs device.
Some devices may be required to be configured multiple times during system 
setup or system development.
As per spec ,the host may write the Configuration Descriptors multiple times if 
bConfigDescrLock attribute is equal to zero.

In my current implementation, bConfigDescrLock attribute is parsed from input 
buffer (via sysfs at runtime) and based on parsed value,
it can be set to one to permanently lock the device configuration.
I can add an extra check to read bConfigDescrLock attribute before writing 
Configuration Descriptors with provisioning data,
as write request shall anyway fail if bConfigDescrLock value is already one.

Regards,
Sayali

-Original Message-
From: Kyuho Choi [mailto:chlrb...@gmail.com] 
Sent: Wednesday, May 23, 2018 7:10 AM
To: Sayali Lokhande 
Cc: subha...@codeaurora.org; c...@codeaurora.org; vivek.gau...@codeaurora.org; 
rna...@codeaurora.org; vinholika...@gmail.com; j...@linux.vnet.ibm.com; 
martin.peter...@oracle.com; asuto...@codeaurora.org; linux-scsi@vger.kernel.org
Subject: Re: [PATCH RFC 0/3] Add ufs provisioning support in driver

Hi Sayali,

Provisioning ufs device need to handle lock/unlock the bConfigdescrlock 
attribute. According  to UFS spec bConfigdescrlock is write once attribute. Are 
we really need this API in kernel?.

On 5/22/18, Sayali Lokhande  wrote:
> This change adds a new API ufshcd_do_config_device() to write 
> configuration descriptor with the provisioning data.
> Sysfs support is added in driver to trigger ufs provisioning at 
> runtime. Provisioning data is parsed from vendor specific provisioning 
> file. This parsed data is passed as a buffer via sysfs to provision 
> ufs device.
>
> Sayali Lokhande (2):
>   scsi: ufs: Add ufs provisioning support
>   scsi: ufs: Add sysfs support for ufs provision
>
> Subhash Jadavani (1):
>   scsi: ufs: set the device reference clock setting
>
>  .../devicetree/bindings/ufs/ufshcd-pltfrm.txt  |   8 +
>  drivers/scsi/ufs/ufs.h |  39 +++
>  drivers/scsi/ufs/ufshcd-pltfrm.c   |  20 ++
>  drivers/scsi/ufs/ufshcd.c  | 383
> +
>  drivers/scsi/ufs/ufshcd.h  |   9 +
>  5 files changed, 459 insertions(+)
>
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora 
> Forum, a Linux Foundation Collaborative Project
>
>

BR,
Kyuho Choi

BUG: scsi/qla2xxx: BUG_ON(blk_queued_rq(req) is triggered in blk_finish_request

[jianchao.wang]

Hi all

Our customer met a panic triggered by BUG_ON in blk_finish_request.
>From the dmesg log, the BUG_ON was triggered after command abort occurred many 
>times.
There is a race condition in the following scenario.

cpu A   cpu B
kworker interrupt
 
scmd_eh_abort_handler()   
  -> scsi_try_to_abort_cmd()
-> qla2xxx_eh_abort()
  -> qla2x00_eh_wait_on_command()   qla2x00_status_entry()   
  -> qla2x00_sp_compl()
-> qla2x00_sp_free_dma()
  -> scsi_queue_insert()  
-> __scsi_queue_insert() 
  -> blk_requeue_request()
-> blk_clear_rq_complete()
-> scsi_done
  -> blk_complete_request
-> blk_mark_rq_complete   
  -> elv_requeue_request()  -> __blk_complete_request()
-> __elv_add_request()
// req will be queued here  
   
BLK_SOFTIRQ
scsi_softirq_done()
  -> scsi_finish_command()
-> scsi_io_completion()
  -> scsi_end_request()
-> 
blk_finish_request()  // BUG_ON(blk_queued_rq(req)) !!!

The issue will not be triggered most of time, because the request is marked as 
complete by timeout path.
So the scsi_done from qla2x00_sp_compl does nothing.
But as the scenario above, if the complete state has been cleaned by 
blk_requeue_request, we will get
the request both requeued and completed, and then BUG_ON(blk_queued_rq(req)) in 
blk_finish_request comes up.

Is there any solution for this in qla2xxx driver side ?

Thanks
Jianchao

Re: [PATCH RFC 0/3] Add ufs provisioning support in driver

[Kyuho Choi]

Hi Sayali,

Provisioning ufs device need to handle lock/unlock the
bConfigdescrlock attribute. According  to UFS spec bConfigdescrlock is
write once attribute. Are we really need this API in kernel?.

On 5/22/18, Sayali Lokhande  wrote:
> This change adds a new API ufshcd_do_config_device() to
> write configuration descriptor with the provisioning data.
> Sysfs support is added in driver to trigger ufs provisioning at
> runtime. Provisioning data is parsed from vendor specific provisioning
> file. This parsed data is passed as a buffer via sysfs to provision
> ufs device.
>
> Sayali Lokhande (2):
>   scsi: ufs: Add ufs provisioning support
>   scsi: ufs: Add sysfs support for ufs provision
>
> Subhash Jadavani (1):
>   scsi: ufs: set the device reference clock setting
>
>  .../devicetree/bindings/ufs/ufshcd-pltfrm.txt  |   8 +
>  drivers/scsi/ufs/ufs.h |  39 +++
>  drivers/scsi/ufs/ufshcd-pltfrm.c   |  20 ++
>  drivers/scsi/ufs/ufshcd.c  | 383
> +
>  drivers/scsi/ufs/ufshcd.h  |   9 +
>  5 files changed, 459 insertions(+)
>
> --
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> a Linux Foundation Collaborative Project
>
>

BR,
Kyuho Choi

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Kees Cook]

On Tue, May 22, 2018 at 4:42 PM, Jens Axboe  wrote:
> On May 22, 2018, at 5:31 PM, Kees Cook  wrote:
>>
>>> On Tue, May 22, 2018 at 12:16 PM, Jens Axboe  wrote:
 On 5/22/18 1:13 PM, Christoph Hellwig wrote:
> On Tue, May 22, 2018 at 01:09:41PM -0600, Jens Axboe wrote:
> I think Martin and Christoph are objecting to moving the code to
> block/scsi_ioctl.h. I don't care too much about where the code is, but
> think it would be nice to have the definitions in a separate header. But
> if they prefer just pulling in all of SCSI for it, well then I guess
> it's pointless to move the header bits. Seems very heavy handed to me,
> though.

 It might be heavy handed for the 3 remaining users of drivers/ide,
>>>
>>> Brutal :-)
>>
>> Heh. I noticed a similar sense buffer use in drivers/cdrom/cdrom.c
>> too. Is this okay under the same considerations?
>
> This is going from somewhat crazy to pretty nuts, imho. I guess in practical 
> terms it doesn’t matter that much, since most folks would be using sr. I 
> still think a split would be vastly superior. Just keep the scsi code in 
> drivers/scsi/ and make it independently selectable.

I had originally tied it to BLK_SCSI_REQUEST. Logically speaking,
sense buffers are part of the request, and the CONFIG work is already
done. This is roughly what I tried to do before, since scsi_ioctl.c is
the only code pulled in for CONFIG_BLK_SCSI_REQUEST:

$ git grep BLK_SCSI_REQUEST | grep Makefile
block/Makefile:obj-$(CONFIG_BLK_SCSI_REQUEST)   += scsi_ioctl.o

Should I move to code to a new drivers/scsi/scsi_sense.c and add it to
drivers/scsi/Makefile as:

obj-$(CONFIG_BLK_SCSI_REQUEST)+= scsi_sense.o

Every place I want to use the code is already covered by
CONFIG_BLK_SCSI_REQUEST, so it seems like I just need to know where to
put the .c file. :P

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Jens Axboe]

On May 22, 2018, at 5:31 PM, Kees Cook  wrote:
> 
>> On Tue, May 22, 2018 at 12:16 PM, Jens Axboe  wrote:
>>> On 5/22/18 1:13 PM, Christoph Hellwig wrote:
 On Tue, May 22, 2018 at 01:09:41PM -0600, Jens Axboe wrote:
 I think Martin and Christoph are objecting to moving the code to
 block/scsi_ioctl.h. I don't care too much about where the code is, but
 think it would be nice to have the definitions in a separate header. But
 if they prefer just pulling in all of SCSI for it, well then I guess
 it's pointless to move the header bits. Seems very heavy handed to me,
 though.
>>> 
>>> It might be heavy handed for the 3 remaining users of drivers/ide,
>> 
>> Brutal :-)
> 
> Heh. I noticed a similar sense buffer use in drivers/cdrom/cdrom.c
> too. Is this okay under the same considerations?

This is going from somewhat crazy to pretty nuts, imho. I guess in practical 
terms it doesn’t matter that much, since most folks would be using sr. I still 
think a split would be vastly superior. Just keep the scsi code in 
drivers/scsi/ and make it independently selectable.

Jens

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Randy Dunlap]

On 05/22/2018 04:39 PM, Kees Cook wrote:
> On Tue, May 22, 2018 at 4:34 PM, Randy Dunlap  wrote:
>> On 05/22/2018 04:31 PM, Kees Cook wrote:
>>> On Tue, May 22, 2018 at 12:16 PM, Jens Axboe  wrote:
 On 5/22/18 1:13 PM, Christoph Hellwig wrote:
> On Tue, May 22, 2018 at 01:09:41PM -0600, Jens Axboe wrote:
>> I think Martin and Christoph are objecting to moving the code to
>> block/scsi_ioctl.h. I don't care too much about where the code is, but
>> think it would be nice to have the definitions in a separate header. But
>> if they prefer just pulling in all of SCSI for it, well then I guess
>> it's pointless to move the header bits. Seems very heavy handed to me,
>> though.
>
> It might be heavy handed for the 3 remaining users of drivers/ide,

 Brutal :-)
>>>
>>> Heh. I noticed a similar sense buffer use in drivers/cdrom/cdrom.c
>>> too. Is this okay under the same considerations?
>>
>> No.  Do not select an entire subsystem.  Use depends on it instead.
> 
> I looked at that first, but it seems it's designed for that. For
> example, "config ATA" already has a "select SCSI".
> 
> It does look fishy, though, since "config SCSI" has a "depends" which
> would be ignored by "select". Luckily, all these uses already do a
> "depends on BLOCK" (directly or indirectly).

Linus has railed against selecting subsystems.  We shouldn't be adding
more IMHO, although it is difficult to get rid of ones that we already have.


-- 
~Randy

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Kees Cook]

On Tue, May 22, 2018 at 4:34 PM, Randy Dunlap  wrote:
> On 05/22/2018 04:31 PM, Kees Cook wrote:
>> On Tue, May 22, 2018 at 12:16 PM, Jens Axboe  wrote:
>>> On 5/22/18 1:13 PM, Christoph Hellwig wrote:
 On Tue, May 22, 2018 at 01:09:41PM -0600, Jens Axboe wrote:
> I think Martin and Christoph are objecting to moving the code to
> block/scsi_ioctl.h. I don't care too much about where the code is, but
> think it would be nice to have the definitions in a separate header. But
> if they prefer just pulling in all of SCSI for it, well then I guess
> it's pointless to move the header bits. Seems very heavy handed to me,
> though.

 It might be heavy handed for the 3 remaining users of drivers/ide,
>>>
>>> Brutal :-)
>>
>> Heh. I noticed a similar sense buffer use in drivers/cdrom/cdrom.c
>> too. Is this okay under the same considerations?
>
> No.  Do not select an entire subsystem.  Use depends on it instead.

I looked at that first, but it seems it's designed for that. For
example, "config ATA" already has a "select SCSI".

It does look fishy, though, since "config SCSI" has a "depends" which
would be ignored by "select". Luckily, all these uses already do a
"depends on BLOCK" (directly or indirectly).

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Randy Dunlap]

On 05/22/2018 04:31 PM, Kees Cook wrote:
> On Tue, May 22, 2018 at 12:16 PM, Jens Axboe  wrote:
>> On 5/22/18 1:13 PM, Christoph Hellwig wrote:
>>> On Tue, May 22, 2018 at 01:09:41PM -0600, Jens Axboe wrote:
 I think Martin and Christoph are objecting to moving the code to
 block/scsi_ioctl.h. I don't care too much about where the code is, but
 think it would be nice to have the definitions in a separate header. But
 if they prefer just pulling in all of SCSI for it, well then I guess
 it's pointless to move the header bits. Seems very heavy handed to me,
 though.
>>>
>>> It might be heavy handed for the 3 remaining users of drivers/ide,
>>
>> Brutal :-)
> 
> Heh. I noticed a similar sense buffer use in drivers/cdrom/cdrom.c
> too. Is this okay under the same considerations?

No.  Do not select an entire subsystem.  Use depends on it instead.


> diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
> index ad9b687a236a..220ff321c102 100644
> --- a/drivers/block/Kconfig
> +++ b/drivers/block/Kconfig
> @@ -79,7 +79,7 @@ config GDROM
> tristate "SEGA Dreamcast GD-ROM drive"
> depends on SH_DREAMCAST
> select CDROM
> -   select BLK_SCSI_REQUEST # only for the generic cdrom code
> +   select SCSI
> help
>   A standard SEGA Dreamcast comes with a modified CD ROM drive called 
> a
>   "GD-ROM" by SEGA to signify it is capable of reading special disks
> @@ -345,7 +345,7 @@ config CDROM_PKTCDVD
> tristate "Packet writing on CD/DVD media (DEPRECATED)"
> depends on !UML
> select CDROM
> -   select BLK_SCSI_REQUEST
> +   select SCSI
> help
>   Note: This driver is deprecated and will be removed from the
>   kernel in the near future!
> diff --git a/drivers/block/paride/Kconfig b/drivers/block/paride/Kconfig
> index f8bd6ef3605a..7fdfcc5eaca5 100644
> --- a/drivers/block/paride/Kconfig
> +++ b/drivers/block/paride/Kconfig
> @@ -27,7 +27,7 @@ config PARIDE_PCD
> tristate "Parallel port ATAPI CD-ROMs"
> depends on PARIDE
> select CDROM
> -   select BLK_SCSI_REQUEST # only for the generic cdrom code
> +   select SCSI
> ---help---
>   This option enables the high-level driver for ATAPI CD-ROM devices
>   connected through a parallel port. If you chose to build PARIDE
> 
>>> but as long as that stuff just keeps working I'd rather worry about
>>> everyone else, and keep the scsi code where it belongs.
>>
>> Fine with me then, hopefully we can some day kill it off.
> 
> I'll send a v2. I found a few other things to fix up (including the
> cdrom.c one).


-- 
~Randy

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Kees Cook]

On Tue, May 22, 2018 at 12:16 PM, Jens Axboe  wrote:
> On 5/22/18 1:13 PM, Christoph Hellwig wrote:
>> On Tue, May 22, 2018 at 01:09:41PM -0600, Jens Axboe wrote:
>>> I think Martin and Christoph are objecting to moving the code to
>>> block/scsi_ioctl.h. I don't care too much about where the code is, but
>>> think it would be nice to have the definitions in a separate header. But
>>> if they prefer just pulling in all of SCSI for it, well then I guess
>>> it's pointless to move the header bits. Seems very heavy handed to me,
>>> though.
>>
>> It might be heavy handed for the 3 remaining users of drivers/ide,
>
> Brutal :-)

Heh. I noticed a similar sense buffer use in drivers/cdrom/cdrom.c
too. Is this okay under the same considerations?

diff --git a/drivers/block/Kconfig b/drivers/block/Kconfig
index ad9b687a236a..220ff321c102 100644
--- a/drivers/block/Kconfig
+++ b/drivers/block/Kconfig
@@ -79,7 +79,7 @@ config GDROM
tristate "SEGA Dreamcast GD-ROM drive"
depends on SH_DREAMCAST
select CDROM
-   select BLK_SCSI_REQUEST # only for the generic cdrom code
+   select SCSI
help
  A standard SEGA Dreamcast comes with a modified CD ROM drive called a
  "GD-ROM" by SEGA to signify it is capable of reading special disks
@@ -345,7 +345,7 @@ config CDROM_PKTCDVD
tristate "Packet writing on CD/DVD media (DEPRECATED)"
depends on !UML
select CDROM
-   select BLK_SCSI_REQUEST
+   select SCSI
help
  Note: This driver is deprecated and will be removed from the
  kernel in the near future!
diff --git a/drivers/block/paride/Kconfig b/drivers/block/paride/Kconfig
index f8bd6ef3605a..7fdfcc5eaca5 100644
--- a/drivers/block/paride/Kconfig
+++ b/drivers/block/paride/Kconfig
@@ -27,7 +27,7 @@ config PARIDE_PCD
tristate "Parallel port ATAPI CD-ROMs"
depends on PARIDE
select CDROM
-   select BLK_SCSI_REQUEST # only for the generic cdrom code
+   select SCSI
---help---
  This option enables the high-level driver for ATAPI CD-ROM devices
  connected through a parallel port. If you chose to build PARIDE

>> but as long as that stuff just keeps working I'd rather worry about
>> everyone else, and keep the scsi code where it belongs.
>
> Fine with me then, hopefully we can some day kill it off.

I'll send a v2. I found a few other things to fix up (including the
cdrom.c one).

Thanks!

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 2/6] scsi: cxlflash: Drop unused sense buffers

[Matthew R. Ochs]

On Tue, May 22, 2018 at 11:15:08AM -0700, Kees Cook wrote:
> This removes the unused sense buffer in read_cap16() and write_same16().
> 
> Signed-off-by: Kees Cook 

Looks good.

Acked-by: Matthew R. Ochs 

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Jens Axboe]

On 5/22/18 1:13 PM, Christoph Hellwig wrote:
> On Tue, May 22, 2018 at 01:09:41PM -0600, Jens Axboe wrote:
>> I think Martin and Christoph are objecting to moving the code to
>> block/scsi_ioctl.h. I don't care too much about where the code is, but
>> think it would be nice to have the definitions in a separate header. But
>> if they prefer just pulling in all of SCSI for it, well then I guess
>> it's pointless to move the header bits. Seems very heavy handed to me,
>> though.
> 
> It might be heavy handed for the 3 remaining users of drivers/ide,

Brutal :-)

> but as long as that stuff just keeps working I'd rather worry about
> everyone else, and keep the scsi code where it belongs.

Fine with me then, hopefully we can some day kill it off.

-- 
Jens Axboe

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Christoph Hellwig]

On Tue, May 22, 2018 at 01:09:41PM -0600, Jens Axboe wrote:
> I think Martin and Christoph are objecting to moving the code to
> block/scsi_ioctl.h. I don't care too much about where the code is, but
> think it would be nice to have the definitions in a separate header. But
> if they prefer just pulling in all of SCSI for it, well then I guess
> it's pointless to move the header bits. Seems very heavy handed to me,
> though.

It might be heavy handed for the 3 remaining users of drivers/ide,
but as long as that stuff just keeps working I'd rather worry about
everyone else, and keep the scsi code where it belongs.

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Jens Axboe]

On 5/22/18 12:59 PM, Kees Cook wrote:
> On Tue, May 22, 2018 at 11:50 AM, Martin K. Petersen
>  wrote:
>>
>> Christoph,
>>
>>> On Tue, May 22, 2018 at 11:15:09AM -0700, Kees Cook wrote:
 Both SCSI and ATAPI share the sense header. In preparation for using the
 struct scsi_sense_hdr more widely, move this into a separate header and
 move the helper function to scsi_ioctl.c which is linked with CONFIG_IDE
 by way of CONFIG_BLK_SCSI_REQUEST.
>>>
>>> Please keep the code where it is and just depend on SCSI on the legacy
>>> ide driver.  No need to do gymnastics just for a legacy case.
>>
>> Yup, I agree.
> 
> Oh, er, this was mainly done at Jens's request. Jens, can you advise?

I think Martin and Christoph are objecting to moving the code to
block/scsi_ioctl.h. I don't care too much about where the code is, but
think it would be nice to have the definitions in a separate header. But
if they prefer just pulling in all of SCSI for it, well then I guess
it's pointless to move the header bits. Seems very heavy handed to me,
though.

-- 
Jens Axboe

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Kees Cook]

On Tue, May 22, 2018 at 11:50 AM, Martin K. Petersen
 wrote:
>
> Christoph,
>
>> On Tue, May 22, 2018 at 11:15:09AM -0700, Kees Cook wrote:
>>> Both SCSI and ATAPI share the sense header. In preparation for using the
>>> struct scsi_sense_hdr more widely, move this into a separate header and
>>> move the helper function to scsi_ioctl.c which is linked with CONFIG_IDE
>>> by way of CONFIG_BLK_SCSI_REQUEST.
>>
>> Please keep the code where it is and just depend on SCSI on the legacy
>> ide driver.  No need to do gymnastics just for a legacy case.
>
> Yup, I agree.

Oh, er, this was mainly done at Jens's request. Jens, can you advise?

-Kees

-- 
Kees Cook
Pixel Security

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Martin K. Petersen]

Christoph,

> On Tue, May 22, 2018 at 11:15:09AM -0700, Kees Cook wrote:
>> Both SCSI and ATAPI share the sense header. In preparation for using the
>> struct scsi_sense_hdr more widely, move this into a separate header and
>> move the helper function to scsi_ioctl.c which is linked with CONFIG_IDE
>> by way of CONFIG_BLK_SCSI_REQUEST.
>
> Please keep the code where it is and just depend on SCSI on the legacy
> ide driver.  No need to do gymnastics just for a legacy case.

Yup, I agree.

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH 2/6] scsi: cxlflash: Drop unused sense buffers

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig 

Re: [PATCH 1/6] ide-cd: Drop unused sense buffers

[Christoph Hellwig]

Looks fine,

Reviewed-by: Christoph Hellwig 

Re: [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Christoph Hellwig]

On Tue, May 22, 2018 at 11:15:09AM -0700, Kees Cook wrote:
> Both SCSI and ATAPI share the sense header. In preparation for using the
> struct scsi_sense_hdr more widely, move this into a separate header and
> move the helper function to scsi_ioctl.c which is linked with CONFIG_IDE
> by way of CONFIG_BLK_SCSI_REQUEST.

Please keep the code where it is and just depend on SCSI on the legacy
ide driver.  No need to do gymnastics just for a legacy case.

[PATCH 1/6] ide-cd: Drop unused sense buffers

[Kees Cook]

This drops unused sense buffers from:

cdrom_eject()
cdrom_read_capacity()
cdrom_read_tocentry()
ide_cd_lockdoor()
ide_cd_read_toc()

Signed-off-by: Kees Cook 
---
 drivers/ide/ide-cd.c   | 36 +++-
 drivers/ide/ide-cd.h   |  2 +-
 drivers/ide/ide-cd_ioctl.c | 34 --
 3 files changed, 28 insertions(+), 44 deletions(-)

diff --git a/drivers/ide/ide-cd.c b/drivers/ide/ide-cd.c
index 5a8e8e3c22cd..1d5b35312e33 100644
--- a/drivers/ide/ide-cd.c
+++ b/drivers/ide/ide-cd.c
@@ -890,8 +890,7 @@ int cdrom_check_status(ide_drive_t *drive, struct 
request_sense *sense)
 }
 
 static int cdrom_read_capacity(ide_drive_t *drive, unsigned long *capacity,
-  unsigned long *sectors_per_frame,
-  struct request_sense *sense)
+  unsigned long *sectors_per_frame)
 {
struct {
__be32 lba;
@@ -908,7 +907,7 @@ static int cdrom_read_capacity(ide_drive_t *drive, unsigned 
long *capacity,
memset(cmd, 0, BLK_MAX_CDB);
cmd[0] = GPCMD_READ_CDVD_CAPACITY;
 
-   stat = ide_cd_queue_pc(drive, cmd, 0, , , sense, 0,
+   stat = ide_cd_queue_pc(drive, cmd, 0, , , NULL, 0,
   RQF_QUIET);
if (stat)
return stat;
@@ -944,8 +943,7 @@ static int cdrom_read_capacity(ide_drive_t *drive, unsigned 
long *capacity,
 }
 
 static int cdrom_read_tocentry(ide_drive_t *drive, int trackno, int msf_flag,
-   int format, char *buf, int buflen,
-   struct request_sense *sense)
+   int format, char *buf, int buflen)
 {
unsigned char cmd[BLK_MAX_CDB];
 
@@ -962,11 +960,11 @@ static int cdrom_read_tocentry(ide_drive_t *drive, int 
trackno, int msf_flag,
if (msf_flag)
cmd[1] = 2;
 
-   return ide_cd_queue_pc(drive, cmd, 0, buf, , sense, 0, 
RQF_QUIET);
+   return ide_cd_queue_pc(drive, cmd, 0, buf, , NULL, 0, RQF_QUIET);
 }
 
 /* Try to read the entire TOC for the disk into our internal buffer. */
-int ide_cd_read_toc(ide_drive_t *drive, struct request_sense *sense)
+int ide_cd_read_toc(ide_drive_t *drive)
 {
int stat, ntracks, i;
struct cdrom_info *info = drive->driver_data;
@@ -996,14 +994,13 @@ int ide_cd_read_toc(ide_drive_t *drive, struct 
request_sense *sense)
 * Check to see if the existing data is still valid. If it is,
 * just return.
 */
-   (void) cdrom_check_status(drive, sense);
+   (void) cdrom_check_status(drive, NULL);
 
if (drive->atapi_flags & IDE_AFLAG_TOC_VALID)
return 0;
 
/* try to get the total cdrom capacity and sector size */
-   stat = cdrom_read_capacity(drive, >capacity, _per_frame,
-  sense);
+   stat = cdrom_read_capacity(drive, >capacity, _per_frame);
if (stat)
toc->capacity = 0x1f;
 
@@ -1016,7 +1013,7 @@ int ide_cd_read_toc(ide_drive_t *drive, struct 
request_sense *sense)
 
/* first read just the header, so we know how long the TOC is */
stat = cdrom_read_tocentry(drive, 0, 1, 0, (char *) >hdr,
-   sizeof(struct atapi_toc_header), sense);
+   sizeof(struct atapi_toc_header));
if (stat)
return stat;
 
@@ -1036,7 +1033,7 @@ int ide_cd_read_toc(ide_drive_t *drive, struct 
request_sense *sense)
  (char *)>hdr,
   sizeof(struct atapi_toc_header) +
   (ntracks + 1) *
-  sizeof(struct atapi_toc_entry), sense);
+  sizeof(struct atapi_toc_entry));
 
if (stat && toc->hdr.first_track > 1) {
/*
@@ -1056,8 +1053,7 @@ int ide_cd_read_toc(ide_drive_t *drive, struct 
request_sense *sense)
   (char *)>hdr,
   sizeof(struct atapi_toc_header) +
   (ntracks + 1) *
-  sizeof(struct atapi_toc_entry),
-  sense);
+  sizeof(struct atapi_toc_entry));
if (stat)
return stat;
 
@@ -1094,7 +1090,7 @@ int ide_cd_read_toc(ide_drive_t *drive, struct 
request_sense *sense)
if (toc->hdr.first_track != CDROM_LEADOUT) {
/* read the multisession information */
stat = cdrom_read_tocentry(drive, 0, 0, 1, (char *)_tmp,
-  sizeof(ms_tmp), sense);
+  sizeof(ms_tmp));
if (stat)
return 

[PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

[Kees Cook]

Both SCSI and ATAPI share the sense header. In preparation for using the
struct scsi_sense_hdr more widely, move this into a separate header and
move the helper function to scsi_ioctl.c which is linked with CONFIG_IDE
by way of CONFIG_BLK_SCSI_REQUEST.

Signed-off-by: Kees Cook 
---
 block/scsi_ioctl.c | 64 ++
 drivers/scsi/scsi_common.c | 64 --
 include/scsi/scsi_cmnd.h   |  1 -
 include/scsi/scsi_common.h | 32 +--
 include/scsi/scsi_sense.h  | 44 ++
 5 files changed, 109 insertions(+), 96 deletions(-)
 create mode 100644 include/scsi/scsi_sense.h

diff --git a/block/scsi_ioctl.c b/block/scsi_ioctl.c
index 60b471f8621b..87ff3cc7a364 100644
--- a/block/scsi_ioctl.c
+++ b/block/scsi_ioctl.c
@@ -728,6 +728,70 @@ void scsi_req_init(struct scsi_request *req)
 }
 EXPORT_SYMBOL(scsi_req_init);
 
+/**
+ * scsi_normalize_sense - normalize main elements from either fixed or
+ * descriptor sense data format into a common format.
+ *
+ * @sense_buffer:  byte array containing sense data returned by device
+ * @sb_len:number of valid bytes in sense_buffer
+ * @sshdr: pointer to instance of structure that common
+ * elements are written to.
+ *
+ * Notes:
+ * The "main elements" from sense data are: response_code, sense_key,
+ * asc, ascq and additional_length (only for descriptor format).
+ *
+ * Typically this function can be called after a device has
+ * responded to a SCSI command with the CHECK_CONDITION status.
+ *
+ * Return value:
+ * true if valid sense data information found, else false;
+ */
+bool scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
+ struct scsi_sense_hdr *sshdr)
+{
+   memset(sshdr, 0, sizeof(struct scsi_sense_hdr));
+
+   if (!sense_buffer || !sb_len)
+   return false;
+
+   sshdr->response_code = (sense_buffer[0] & 0x7f);
+
+   if (!scsi_sense_valid(sshdr))
+   return false;
+
+   if (sshdr->response_code >= 0x72) {
+   /*
+* descriptor format
+*/
+   if (sb_len > 1)
+   sshdr->sense_key = (sense_buffer[1] & 0xf);
+   if (sb_len > 2)
+   sshdr->asc = sense_buffer[2];
+   if (sb_len > 3)
+   sshdr->ascq = sense_buffer[3];
+   if (sb_len > 7)
+   sshdr->additional_length = sense_buffer[7];
+   } else {
+   /*
+* fixed format
+*/
+   if (sb_len > 2)
+   sshdr->sense_key = (sense_buffer[2] & 0xf);
+   if (sb_len > 7) {
+   sb_len = (sb_len < (sense_buffer[7] + 8)) ?
+sb_len : (sense_buffer[7] + 8);
+   if (sb_len > 12)
+   sshdr->asc = sense_buffer[12];
+   if (sb_len > 13)
+   sshdr->ascq = sense_buffer[13];
+   }
+   }
+
+   return true;
+}
+EXPORT_SYMBOL(scsi_normalize_sense);
+
 static int __init blk_scsi_ioctl_init(void)
 {
blk_set_cmd_filter_defaults(_default_cmd_filter);
diff --git a/drivers/scsi/scsi_common.c b/drivers/scsi/scsi_common.c
index 90349498f686..8621a07cb967 100644
--- a/drivers/scsi/scsi_common.c
+++ b/drivers/scsi/scsi_common.c
@@ -116,70 +116,6 @@ void int_to_scsilun(u64 lun, struct scsi_lun *scsilun)
 }
 EXPORT_SYMBOL(int_to_scsilun);
 
-/**
- * scsi_normalize_sense - normalize main elements from either fixed or
- * descriptor sense data format into a common format.
- *
- * @sense_buffer:  byte array containing sense data returned by device
- * @sb_len:number of valid bytes in sense_buffer
- * @sshdr: pointer to instance of structure that common
- * elements are written to.
- *
- * Notes:
- * The "main elements" from sense data are: response_code, sense_key,
- * asc, ascq and additional_length (only for descriptor format).
- *
- * Typically this function can be called after a device has
- * responded to a SCSI command with the CHECK_CONDITION status.
- *
- * Return value:
- * true if valid sense data information found, else false;
- */
-bool scsi_normalize_sense(const u8 *sense_buffer, int sb_len,
- struct scsi_sense_hdr *sshdr)
-{
-   memset(sshdr, 0, sizeof(struct scsi_sense_hdr));
-
-   if (!sense_buffer || !sb_len)
-   return false;
-
-   sshdr->response_code = (sense_buffer[0] & 0x7f);
-
-   if (!scsi_sense_valid(sshdr))
-   return false;
-
-   if (sshdr->response_code >= 0x72) {
-   /*
-* descriptor format
-*/
-   if 

[PATCH 0/6] block: Consolidate scsi sense buffer usage

[Kees Cook]

This is a follow-up to commit f7068114d45e ("sr: pass down correctly
sized SCSI sense buffer") which further cleans up and removes needless
sense character array buffers and "struct request_sense" usage in favor
of the common "struct scsi_sense_hdr".

First, drop a bunch of unused sense buffers:
 [PATCH 1/6] ide-cd: Drop unused sense buffers
 [PATCH 2/6] scsi: cxlflash: Drop unused sense buffers

Next, split out struct scsi_sense_hdr:
 [PATCH 3/6] block: Create scsi_sense.h for SCSI and ATAPI

Then move all request_sense usage to scsi_sense_hdr:
 [PATCH 4/6] block: Consolidate scsi sense buffer usage

Finally add a build-time check to make sure we don't pass bad buffer sizes:
 [PATCH 5/6] libata-scsi: Move sense buffers onto stack
 [PATCH 6/6] scsi: Check sense buffer size at build time

-Kees

[PATCH 2/6] scsi: cxlflash: Drop unused sense buffers

[Kees Cook]

This removes the unused sense buffer in read_cap16() and write_same16().

Signed-off-by: Kees Cook 
---
 drivers/scsi/cxlflash/superpipe.c | 8 ++--
 drivers/scsi/cxlflash/vlun.c  | 7 ++-
 2 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/drivers/scsi/cxlflash/superpipe.c 
b/drivers/scsi/cxlflash/superpipe.c
index 2fe79df5c73c..59b9f2023748 100644
--- a/drivers/scsi/cxlflash/superpipe.c
+++ b/drivers/scsi/cxlflash/superpipe.c
@@ -324,7 +324,6 @@ static int read_cap16(struct scsi_device *sdev, struct 
llun_info *lli)
struct scsi_sense_hdr sshdr;
u8 *cmd_buf = NULL;
u8 *scsi_cmd = NULL;
-   u8 *sense_buf = NULL;
int rc = 0;
int result = 0;
int retry_cnt = 0;
@@ -333,8 +332,7 @@ static int read_cap16(struct scsi_device *sdev, struct 
llun_info *lli)
 retry:
cmd_buf = kzalloc(CMD_BUFSIZE, GFP_KERNEL);
scsi_cmd = kzalloc(MAX_COMMAND_SIZE, GFP_KERNEL);
-   sense_buf = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_KERNEL);
-   if (unlikely(!cmd_buf || !scsi_cmd || !sense_buf)) {
+   if (unlikely(!cmd_buf || !scsi_cmd)) {
rc = -ENOMEM;
goto out;
}
@@ -349,7 +347,7 @@ static int read_cap16(struct scsi_device *sdev, struct 
llun_info *lli)
/* Drop the ioctl read semahpore across lengthy call */
up_read(>ioctl_rwsem);
result = scsi_execute(sdev, scsi_cmd, DMA_FROM_DEVICE, cmd_buf,
- CMD_BUFSIZE, sense_buf, , to, CMD_RETRIES,
+ CMD_BUFSIZE, NULL, , to, CMD_RETRIES,
  0, 0, NULL);
down_read(>ioctl_rwsem);
rc = check_state(cfg);
@@ -380,7 +378,6 @@ static int read_cap16(struct scsi_device *sdev, struct 
llun_info *lli)
if (retry_cnt++ < 1) {
kfree(cmd_buf);
kfree(scsi_cmd);
-   kfree(sense_buf);
goto retry;
}
}
@@ -411,7 +408,6 @@ static int read_cap16(struct scsi_device *sdev, struct 
llun_info *lli)
 out:
kfree(cmd_buf);
kfree(scsi_cmd);
-   kfree(sense_buf);
 
dev_dbg(dev, "%s: maxlba=%lld blklen=%d rc=%d\n",
__func__, gli->max_lba, gli->blk_len, rc);
diff --git a/drivers/scsi/cxlflash/vlun.c b/drivers/scsi/cxlflash/vlun.c
index 5deef57a7834..e7e9b2f2ad21 100644
--- a/drivers/scsi/cxlflash/vlun.c
+++ b/drivers/scsi/cxlflash/vlun.c
@@ -425,7 +425,6 @@ static int write_same16(struct scsi_device *sdev,
 {
u8 *cmd_buf = NULL;
u8 *scsi_cmd = NULL;
-   u8 *sense_buf = NULL;
int rc = 0;
int result = 0;
u64 offset = lba;
@@ -439,8 +438,7 @@ static int write_same16(struct scsi_device *sdev,
 
cmd_buf = kzalloc(CMD_BUFSIZE, GFP_KERNEL);
scsi_cmd = kzalloc(MAX_COMMAND_SIZE, GFP_KERNEL);
-   sense_buf = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_KERNEL);
-   if (unlikely(!cmd_buf || !scsi_cmd || !sense_buf)) {
+   if (unlikely(!cmd_buf || !scsi_cmd)) {
rc = -ENOMEM;
goto out;
}
@@ -456,7 +454,7 @@ static int write_same16(struct scsi_device *sdev,
/* Drop the ioctl read semahpore across lengthy call */
up_read(>ioctl_rwsem);
result = scsi_execute(sdev, scsi_cmd, DMA_TO_DEVICE, cmd_buf,
- CMD_BUFSIZE, sense_buf, NULL, to,
+ CMD_BUFSIZE, NULL, NULL, to,
  CMD_RETRIES, 0, 0, NULL);
down_read(>ioctl_rwsem);
rc = check_state(cfg);
@@ -481,7 +479,6 @@ static int write_same16(struct scsi_device *sdev,
 out:
kfree(cmd_buf);
kfree(scsi_cmd);
-   kfree(sense_buf);
dev_dbg(dev, "%s: returning rc=%d\n", __func__, rc);
return rc;
 }
-- 
2.17.0

[PATCH 5/6] libata-scsi: Move sense buffers onto stack

[Kees Cook]

Instead of dynamically allocating the sense buffers, put them on the
stack so that future compile-time sizeof() checks will be able to see
their buffer length.

Signed-off-by: Kees Cook 
---
 drivers/ata/libata-scsi.c | 18 ++
 1 file changed, 6 insertions(+), 12 deletions(-)

diff --git a/drivers/ata/libata-scsi.c b/drivers/ata/libata-scsi.c
index 89a9d4a2efc8..3a43d3a1ce2d 100644
--- a/drivers/ata/libata-scsi.c
+++ b/drivers/ata/libata-scsi.c
@@ -597,8 +597,9 @@ static int ata_get_identity(struct ata_port *ap, struct 
scsi_device *sdev,
 int ata_cmd_ioctl(struct scsi_device *scsidev, void __user *arg)
 {
int rc = 0;
+   u8 sensebuf[SCSI_SENSE_BUFFERSIZE];
u8 scsi_cmd[MAX_COMMAND_SIZE];
-   u8 args[4], *argbuf = NULL, *sensebuf = NULL;
+   u8 args[4], *argbuf = NULL;
int argsize = 0;
enum dma_data_direction data_dir;
struct scsi_sense_hdr sshdr;
@@ -610,10 +611,7 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user 
*arg)
if (copy_from_user(args, arg, sizeof(args)))
return -EFAULT;
 
-   sensebuf = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_NOIO);
-   if (!sensebuf)
-   return -ENOMEM;
-
+   memset(sensebuf, 0, sizeof(sensebuf));
memset(scsi_cmd, 0, sizeof(scsi_cmd));
 
if (args[3]) {
@@ -685,7 +683,6 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user 
*arg)
 && copy_to_user(arg + sizeof(args), argbuf, argsize))
rc = -EFAULT;
 error:
-   kfree(sensebuf);
kfree(argbuf);
return rc;
 }
@@ -704,8 +701,9 @@ int ata_cmd_ioctl(struct scsi_device *scsidev, void __user 
*arg)
 int ata_task_ioctl(struct scsi_device *scsidev, void __user *arg)
 {
int rc = 0;
+   u8 sensebuf[SCSI_SENSE_BUFFERSIZE];
u8 scsi_cmd[MAX_COMMAND_SIZE];
-   u8 args[7], *sensebuf = NULL;
+   u8 args[7];
struct scsi_sense_hdr sshdr;
int cmd_result;
 
@@ -715,10 +713,7 @@ int ata_task_ioctl(struct scsi_device *scsidev, void 
__user *arg)
if (copy_from_user(args, arg, sizeof(args)))
return -EFAULT;
 
-   sensebuf = kzalloc(SCSI_SENSE_BUFFERSIZE, GFP_NOIO);
-   if (!sensebuf)
-   return -ENOMEM;
-
+   memset(sensebuf, 0, sizeof(sensebuf));
memset(scsi_cmd, 0, sizeof(scsi_cmd));
scsi_cmd[0]  = ATA_16;
scsi_cmd[1]  = (3 << 1); /* Non-data */
@@ -769,7 +764,6 @@ int ata_task_ioctl(struct scsi_device *scsidev, void __user 
*arg)
}
 
  error:
-   kfree(sensebuf);
return rc;
 }
 
-- 
2.17.0

[PATCH 6/6] scsi: Check sense buffer size at build time

[Kees Cook]

To avoid introducing problems like those fixed in commit f7068114d45e
("sr: pass down correctly sized SCSI sense buffer"), this creates a macro
wrapper for scsi_execute() that verifies the size of the sense buffer
similar to what was done for command string sizes in commit 3756f6401c30
("exec: avoid gcc-8 warning for get_task_comm").

Another solution could be to add another argument to scsi_execute(),
but this function already takes a lot of arguments and Jens was not fond
of that approach. As there was only a pair of dynamically allocated sense
buffers, this also moves those 96 bytes onto the stack to avoid triggering
the sizeof() check.

Signed-off-by: Kees Cook 
---
 drivers/scsi/scsi_lib.c|  6 +++---
 include/scsi/scsi_device.h | 12 +++-
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index e9b4f279d29c..718c2bec4516 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -238,7 +238,7 @@ void scsi_queue_insert(struct scsi_cmnd *cmd, int reason)
 
 
 /**
- * scsi_execute - insert request and wait for the result
+ * __scsi_execute - insert request and wait for the result
  * @sdev:  scsi device
  * @cmd:   scsi command
  * @data_direction: data direction
@@ -255,7 +255,7 @@ void scsi_queue_insert(struct scsi_cmnd *cmd, int reason)
  * Returns the scsi_cmnd result field if a command was executed, or a negative
  * Linux error code if we didn't get that far.
  */
-int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
+int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
 int data_direction, void *buffer, unsigned bufflen,
 unsigned char *sense, struct scsi_sense_hdr *sshdr,
 int timeout, int retries, u64 flags, req_flags_t rq_flags,
@@ -309,7 +309,7 @@ int scsi_execute(struct scsi_device *sdev, const unsigned 
char *cmd,
 
return ret;
 }
-EXPORT_SYMBOL(scsi_execute);
+EXPORT_SYMBOL(__scsi_execute);
 
 /*
  * Function:scsi_init_cmd_errh()
diff --git a/include/scsi/scsi_device.h b/include/scsi/scsi_device.h
index 7ae177c8e399..1bb87b6c0ad2 100644
--- a/include/scsi/scsi_device.h
+++ b/include/scsi/scsi_device.h
@@ -426,11 +426,21 @@ extern const char *scsi_device_state_name(enum 
scsi_device_state);
 extern int scsi_is_sdev_device(const struct device *);
 extern int scsi_is_target_device(const struct device *);
 extern void scsi_sanitize_inquiry_string(unsigned char *s, int len);
-extern int scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
+extern int __scsi_execute(struct scsi_device *sdev, const unsigned char *cmd,
int data_direction, void *buffer, unsigned bufflen,
unsigned char *sense, struct scsi_sense_hdr *sshdr,
int timeout, int retries, u64 flags,
req_flags_t rq_flags, int *resid);
+/* Make sure any sense buffer is the correct size. */
+#define scsi_execute(sdev, cmd, data_direction, buffer, bufflen, sense,
\
+sshdr, timeout, retries, flags, rq_flags, resid)   \
+({ \
+   BUILD_BUG_ON((sense) != NULL && \
+sizeof(sense) != SCSI_SENSE_BUFFERSIZE);   \
+   __scsi_execute(sdev, cmd, data_direction, buffer, bufflen,  \
+  sense, sshdr, timeout, retries, flags, rq_flags, \
+  resid);  \
+})
 static inline int scsi_execute_req(struct scsi_device *sdev,
const unsigned char *cmd, int data_direction, void *buffer,
unsigned bufflen, struct scsi_sense_hdr *sshdr, int timeout,
-- 
2.17.0

[PATCH 4/6] block: Consolidate scsi sense buffer usage

[Kees Cook]

There is a lot of needless struct request_sense usage in the CDROM
code. These can all be struct scsi_sense_hdr instead, to avoid any
confusion over their respective structure sizes. This patch is a lot
of noise changing "sense" to "sshdr", but the final code is more
readable to distinguish between "sense" meaning "struct request_sense"
and "sshdr" meaning "struct scsi_sense_hdr".

Signed-off-by: Kees Cook 
---
 drivers/block/pktcdvd.c| 36 ++--
 drivers/cdrom/cdrom.c  | 22 +++---
 drivers/ide/ide-cd.c   | 11 ++-
 drivers/ide/ide-cd.h   |  5 +++--
 drivers/ide/ide-cd_ioctl.c | 30 +++---
 drivers/scsi/sr_ioctl.c| 22 +-
 include/linux/cdrom.h  |  3 ++-
 7 files changed, 64 insertions(+), 65 deletions(-)

diff --git a/drivers/block/pktcdvd.c b/drivers/block/pktcdvd.c
index c61d20c9f3f8..f91c9f85e29d 100644
--- a/drivers/block/pktcdvd.c
+++ b/drivers/block/pktcdvd.c
@@ -748,13 +748,13 @@ static const char *sense_key_string(__u8 index)
 static void pkt_dump_sense(struct pktcdvd_device *pd,
   struct packet_command *cgc)
 {
-   struct request_sense *sense = cgc->sense;
+   struct scsi_sense_hdr *sshdr = cgc->sshdr;
 
-   if (sense)
+   if (sshdr)
pkt_err(pd, "%*ph - sense %02x.%02x.%02x (%s)\n",
CDROM_PACKET_SIZE, cgc->cmd,
-   sense->sense_key, sense->asc, sense->ascq,
-   sense_key_string(sense->sense_key));
+   sshdr->sense_key, sshdr->asc, sshdr->ascq,
+   sense_key_string(sshdr->sense_key));
else
pkt_err(pd, "%*ph - no sense\n", CDROM_PACKET_SIZE, cgc->cmd);
 }
@@ -787,11 +787,11 @@ static noinline_for_stack int pkt_set_speed(struct 
pktcdvd_device *pd,
unsigned write_speed, unsigned read_speed)
 {
struct packet_command cgc;
-   struct request_sense sense;
+   struct scsi_sense_hdr sshdr;
int ret;
 
init_cdrom_command(, NULL, 0, CGC_DATA_NONE);
-   cgc.sense = 
+   cgc.sshdr = 
cgc.cmd[0] = GPCMD_SET_SPEED;
cgc.cmd[2] = (read_speed >> 8) & 0xff;
cgc.cmd[3] = read_speed & 0xff;
@@ -1645,7 +1645,7 @@ static noinline_for_stack int pkt_get_last_written(struct 
pktcdvd_device *pd,
 static noinline_for_stack int pkt_set_write_settings(struct pktcdvd_device *pd)
 {
struct packet_command cgc;
-   struct request_sense sense;
+   struct scsi_sense_hdr sshdr;
write_param_page *wp;
char buffer[128];
int ret, size;
@@ -1656,7 +1656,7 @@ static noinline_for_stack int 
pkt_set_write_settings(struct pktcdvd_device *pd)
 
memset(buffer, 0, sizeof(buffer));
init_cdrom_command(, buffer, sizeof(*wp), CGC_DATA_READ);
-   cgc.sense = 
+   cgc.sshdr = 
if ((ret = pkt_mode_sense(pd, , GPMODE_WRITE_PARMS_PAGE, 0))) {
pkt_dump_sense(pd, );
return ret;
@@ -1671,7 +1671,7 @@ static noinline_for_stack int 
pkt_set_write_settings(struct pktcdvd_device *pd)
 * now get it all
 */
init_cdrom_command(, buffer, size, CGC_DATA_READ);
-   cgc.sense = 
+   cgc.sshdr = 
if ((ret = pkt_mode_sense(pd, , GPMODE_WRITE_PARMS_PAGE, 0))) {
pkt_dump_sense(pd, );
return ret;
@@ -1905,12 +1905,12 @@ static noinline_for_stack int pkt_write_caching(struct 
pktcdvd_device *pd,
int set)
 {
struct packet_command cgc;
-   struct request_sense sense;
+   struct scsi_sense_hdr sshdr;
unsigned char buf[64];
int ret;
 
init_cdrom_command(, buf, sizeof(buf), CGC_DATA_READ);
-   cgc.sense = 
+   cgc.sshdr = 
cgc.buflen = pd->mode_offset + 12;
 
/*
@@ -1950,14 +1950,14 @@ static noinline_for_stack int pkt_get_max_speed(struct 
pktcdvd_device *pd,
unsigned *write_speed)
 {
struct packet_command cgc;
-   struct request_sense sense;
+   struct scsi_sense_hdr sshdr;
unsigned char buf[256+18];
unsigned char *cap_buf;
int ret, offset;
 
cap_buf = [sizeof(struct mode_page_header) + pd->mode_offset];
init_cdrom_command(, buf, sizeof(buf), CGC_DATA_UNKNOWN);
-   cgc.sense = 
+   cgc.sshdr = 
 
ret = pkt_mode_sense(pd, , GPMODE_CAPABILITIES_PAGE, 0);
if (ret) {
@@ -2011,13 +2011,13 @@ static noinline_for_stack int pkt_media_speed(struct 
pktcdvd_device *pd,
unsigned *speed)
 {
struct packet_command cgc;
-   struct request_sense sense;
+   struct scsi_sense_hdr sshdr;
unsigned char buf[64];
unsigned int size, st, sp;
int ret;
 
init_cdrom_command(, buf, 2, 

Re: [PATCH 1/2] libsas: remove irq save in sas_ata_qc_issue()

[Sebastian Andrzej Siewior]

On 2018-05-18 14:31:27 [+0100], John Garry wrote:
> On 04/05/2018 15:50, Sebastian Andrzej Siewior wrote:
> > Since commit 312d3e56119a ("[SCSI] libsas: remove ata_port.lock
> > management duties from lldds") the sas_ata_qc_issue() function unlocks
> > the ata_port.lock and disables interrupts before doing so.
> > That lock is always taken with disabled interrupts so at this point, the
> > interrupts are already disabled. There is no need to disable the
> > interrupts before the unlock operation because they are already
> > disabled.
> 
> Is the only caller ata_qc_issue(), which has spin_lock_irqsave(host lock)
> enabled?

I don't understand the question. 

> > Restoring the interrupt state later does not change anything because
> > they were disabled and remain disabled. Therefore remove the operations
> > which do not change the behaviour.
> > 
> > Signed-off-by: Sebastian Andrzej Siewior 
> > ---
> >  drivers/scsi/libsas/sas_ata.c | 3 ---
> >  1 file changed, 3 deletions(-)
> > 
> > diff --git a/drivers/scsi/libsas/sas_ata.c b/drivers/scsi/libsas/sas_ata.c
> > index 0cc1567eacc1..bc5d917ff643 100644
> > --- a/drivers/scsi/libsas/sas_ata.c
> > +++ b/drivers/scsi/libsas/sas_ata.c
> > @@ -176,7 +176,6 @@ static void sas_ata_task_done(struct sas_task *task)
> > 
> >  static unsigned int sas_ata_qc_issue(struct ata_queued_cmd *qc)
> >  {
> > -   unsigned long flags;
> > struct sas_task *task;
> > struct scatterlist *sg;
> > int ret = AC_ERR_SYSTEM;
> > @@ -190,7 +189,6 @@ static unsigned int sas_ata_qc_issue(struct 
> > ata_queued_cmd *qc)
> > /* TODO: audit callers to ensure they are ready for qc_issue to
> >  * unconditionally re-enable interrupts
> >  */
> 
> Is the "TODO" comment still relevant?

can't tell. The interrupts are never enabled during the unlock.

> cheers,
> 
> > -   local_irq_save(flags);
> > spin_unlock(ap->lock);
> > 
> > /* If the device fell off, no sense in issuing commands */
> > @@ -252,7 +250,6 @@ static unsigned int sas_ata_qc_issue(struct 
> > ata_queued_cmd *qc)
> > 
> >   out:
> > spin_lock(ap->lock);
> > -   local_irq_restore(flags);
> > return ret;
> >  }

Sebastian

Re: [PATCH RFC] sr: mark the device as changed when burning a CD

[Jens Axboe]

On 5/22/18 3:15 AM, Maurizio Lombardi wrote:
> Some of our customers complain that, after burning a CD, it's not possible
> to proceed to mount it without ejecting it first.
> 
> The problem is that the sr driver caches some information about the CD-ROM
> and doesn't update them after burning it, leading to a failure
> when the isofs filesystem tries to read the superblock.
> 
> This patch modifies the sr driver so it detects the SG_IO
> write ioctls and marks the device as "changed".
> As a result, at the next open() syscall, the revalidate_block()
> callback will be executed and the sr driver will update its cached info.
> 
> Is this patch acceptable to you? Do you have different ideas?

It's been many years, but back in the day the program writing the cd
would eject the disc once done. This of course forces a reload of
the toc and clearing of the flag. What program is this? Seems like
it should probably eject when it's done.

There are many commands that will indicate writing to the device, but
not cause anything that should force a reload. A mode select comes to
mind.

-- 
Jens Axboe

[PATCH RFC] sr: mark the device as changed when burning a CD

[Maurizio Lombardi]

Some of our customers complain that, after burning a CD, it's not possible
to proceed to mount it without ejecting it first.

The problem is that the sr driver caches some information about the CD-ROM
and doesn't update them after burning it, leading to a failure
when the isofs filesystem tries to read the superblock.

This patch modifies the sr driver so it detects the SG_IO
write ioctls and marks the device as "changed".
As a result, at the next open() syscall, the revalidate_block()
callback will be executed and the sr driver will update its cached info.

Is this patch acceptable to you? Do you have different ideas?

Maurizio Lombardi (1):
  sr: mark device as changed when performing a write SG_IO operation

 drivers/scsi/sr.c | 8 
 1 file changed, 8 insertions(+)

-- 
Maurizio Lombardi

[PATCH] sr: mark device as changed when performing a write SG_IO operation

[Maurizio Lombardi]

Signed-off-by: Maurizio Lombardi 
---
 drivers/scsi/sr.c | 8 
 1 file changed, 8 insertions(+)

diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 3f3cb72..e64311d 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -49,6 +49,7 @@
 #include 
 
 #include 
+#include 
 #include 
 #include 
 #include 
@@ -553,6 +554,7 @@ static int sr_block_ioctl(struct block_device *bdev, 
fmode_t mode, unsigned cmd,
struct scsi_cd *cd = scsi_cd(bdev->bd_disk);
struct scsi_device *sdev = cd->device;
void __user *argp = (void __user *)arg;
+   struct sg_io_hdr hdr;
int ret;
 
mutex_lock(_mutex);
@@ -571,6 +573,12 @@ static int sr_block_ioctl(struct block_device *bdev, 
fmode_t mode, unsigned cmd,
case SCSI_IOCTL_GET_BUS_NUMBER:
ret = scsi_ioctl(sdev, cmd, argp);
goto out;
+   case SG_IO:
+   if (copy_from_user(, argp, sizeof(hdr)))
+   break;
+   if (hdr.dxfer_direction == SG_DXFER_TO_DEV)
+   sdev->changed = 1;
+   break;
}
 
ret = cdrom_ioctl(>cdi, bdev, mode, cmd, arg);
-- 
Maurizio Lombardi

Re: [PATCH v2] scsi_transport_srp: Fix shost to rport translation

[Johannes Thumshirn]

Looks good,
Reviewed-by: Johannes Thumshirn 
-- 
Johannes Thumshirn  Storage
jthumsh...@suse.de+49 911 74053 689
SUSE LINUX GmbH, Maxfeldstr. 5, 90409 Nürnberg
GF: Felix Imendörffer, Jane Smithard, Graham Norton
HRB 21284 (AG Nürnberg)
Key fingerprint = EC38 9CAB C2C4 F25D 8600 D0D0 0393 969D 2D76 0850