[PATCH] scsi_lib: sanitize++ in progress

[Douglas Gilbert]

A patch titled: "scsi: sd: improved drive sanitize error handling" by
Mahesh Rajashekhara on 20180417 has been accepted into the kernel. It
may not be sufficient, especially if the SCSI SANITIZE command is sent
via the bsg or sg pass-throughs, since they don't use the sd driver.

Add "Sanitize in progress" plus some other recent "... in progress"
additional sense codes into the scsi mid-level so they are treated in
a similar fashion to "Format in progress".

Signed-off-by: Douglas Gilbert 
---
 drivers/scsi/scsi_lib.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/drivers/scsi/scsi_lib.c b/drivers/scsi/scsi_lib.c
index e9b4f279d29c..9df5fbdbc854 100644
--- a/drivers/scsi/scsi_lib.c
+++ b/drivers/scsi/scsi_lib.c
@@ -985,6 +985,10 @@ void scsi_io_completion(struct scsi_cmnd *cmd, unsigned 
int good_bytes)
case 0x08: /* Long write in progress */
case 0x09: /* self test in progress */
case 0x14: /* space allocation in progress */
+   case 0x1a: /* start stop unit in progress */
+   case 0x1b: /* sanitize in progress */
+   case 0x1d: /* configuration in progress */
+   case 0x24: /* depopulation in progress */
action = ACTION_DELAYED_RETRY;
break;
default:
-- 
2.17.0

Re: [PATCH] scsi: sg: fix a missing-check bug

[Wenwen Wang]

On Mon, May 7, 2018 at 12:13 AM, Douglas Gilbert  wrote:
> On 2018-05-05 11:21 PM, Wenwen Wang wrote:
>>
>> In sg_write(), the opcode of the command is firstly copied from the
>> userspace pointer 'buf' and saved to the kernel variable 'opcode', using
>> the __get_user() function. The size of the command, i.e., 'cmd_size' is
>> then calculated based on the 'opcode'. After that, the whole command,
>> including the opcode, is copied again from 'buf' using the
>> __copy_from_user() function and saved to 'cmnd'. Finally, the function
>>   sg_common_write() is invoked to process 'cmnd'. Given that the 'buf'
>> pointer resides in userspace, a malicious userspace process can race to
>> change the opcode of the command between the two copies. That means, the
>> opcode indicated by the variable 'opcode' could be different from the
>> opcode in 'cmnd'. This can cause inconsistent data in 'cmnd' and
>> potential logical errors in the function sg_common_write(), as it needs to
>> work on 'cmnd'.
>>
>> This patch reuses the opcode obtained in the first copy and only copies
>> the
>> remaining part of the command from userspace.
>>
>> Signed-off-by: Wenwen Wang 
>> ---
>>   drivers/scsi/sg.c | 3 ++-
>>   1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
>> index c198b963..0ad8106 100644
>> --- a/drivers/scsi/sg.c
>> +++ b/drivers/scsi/sg.c
>> @@ -657,7 +657,8 @@ sg_write(struct file *filp, const char __user *buf,
>> size_t count, loff_t * ppos)
>> hp->flags = input_size; /* structure abuse ... */
>> hp->pack_id = old_hdr.pack_id;
>> hp->usr_ptr = NULL;
>> -   if (__copy_from_user(cmnd, buf, cmd_size))
>> +   cmnd[0] = opcode;
>> +   if (__copy_from_user(cmnd + 1, buf + 1, cmd_size - 1))
>> return -EFAULT;
>> /*
>>  * SG_DXFER_TO_FROM_DEV is functionally equivalent to
>> SG_DXFER_FROM_DEV,
>>
>
> That is in the deprecated "v2" part of the sg driver (for around 15 years).
> There are lots more interesting races with that interface than that one
> described above. My guess is that all system calls would be susceptible
> to playing around with a buffer being passed to or from the OS by a thread
> other than the one doing the system call, during that call. Surely no Unix
> like OS gives any security guarantees to a thread being attacked by a
> malevolent thread in the same process!
>
> My question is did this actually cause to program to fail; or is it
> something
> that a sanity checker flagged?

This is detected by a static analysis tool. But, based on our manual
investigation, it can cause program failure. So it is better to fix
it.

>
> Also wouldn't it be better just to return an error such as EINVAL if
> opcode != command[0]  ?

I can revise this patch to return EINVAL if the opcode is not as
expected, and resubmit it.

Thanks!

Wenwen

[PATCH] aic94xx: don't return zero on failure paths in aic94xx_init()

[Alexey Khoroshilov]

If sas_domain_attach_transport() fails in aic94xx_init(),
it breaks off initialization, deallocates all resources, but returns zero.

The patch adds -ENOMEM as return value in this case.

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Peter Melnichenko 
Signed-off-by: Alexey Khoroshilov 
---
 drivers/scsi/aic94xx/aic94xx_init.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/scsi/aic94xx/aic94xx_init.c 
b/drivers/scsi/aic94xx/aic94xx_init.c
index 6c838865ac5a..4a4746cc6745 100644
--- a/drivers/scsi/aic94xx/aic94xx_init.c
+++ b/drivers/scsi/aic94xx/aic94xx_init.c
@@ -1030,8 +1030,10 @@ static int __init aic94xx_init(void)
 
aic94xx_transport_template =
sas_domain_attach_transport(_transport_functions);
-   if (!aic94xx_transport_template)
+   if (!aic94xx_transport_template) {
+   err = -ENOMEM;
goto out_destroy_caches;
+   }
 
err = pci_register_driver(_pci_driver);
if (err)
-- 
2.7.4

Re: [PATCH v4] target: transport should handle st FM/EOM/ILI reads

[Martin K. Petersen]

Lee,

> When a tape drive is exported via LIO using the pscsi module, a read
> that requests more bytes per block than the tape can supply returns an
> empty buffer. This is because the pscsi pass-through target module
> sees the "ILI" illegal length bit set and thinks there is no reason to
> return the data.

Applied to 4.18/scsi-queue. Thank you!

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH] scsi: snic: fix a couple of spelling mistakes: "COMPLETE"

[Martin K. Petersen]

Colin,

> Trivial fix to spelling mistakes/typos:
> "SNIC_IOREQ_ABTS_COMPELTE" -> "SNIC_IOREQ_ABTS_COMPLETE"
> "SNIC_IOREQ_LR_COMPELTE"   -> "SNIC_IOREQ_LR_COMPLETE"
> "SNIC_IOREQ_CMD_COMPELTE"  -> "SNIC_IOREQ_CMD_COMPLETE"

Applied to 4.18/scsi-queue. Thanks!

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH] scsi: qlogicpti: Fix an error handling path in 'qpti_sbus_probe()'

[Martin K. Petersen]

Christophe,

> The 'free_irq()' call is not at the right place in the error handling
> path.  The changed order has been introduced in commit 3d4253d9afab
> ("[SCSI] qlogicpti: Convert to new SBUS device framework.")

Applied to 4.18/scsi-queue. Thanks!

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH v2 00/10] ufshcd optimizations and fixes

[Martin K. Petersen]

Asutosh,

> This patch set has a bunch of optimizations for UFS HCI.

Applied patches 4-8 and 10 to 4.18/scsi-queue.

Patches 1-3 were dropped on request and patch 9 needs review.

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH] scsi: sg: allocate with __GFP_ZERO in sg_build_indirect()

[Christoph Hellwig]

On Fri, May 18, 2018 at 04:23:18PM +0200, Alexander Potapenko wrote:
> This shall help avoid copying uninitialized memory to the userspace
> when calling ioctl(fd, SG_IO) with an empty command.

Looks good,

Reviewed-by: Christoph Hellwig 

Re: [PATCH] scsi: dpt_i2o: Remove VLA usage

[Martin K. Petersen]

Kees,

>> On the quest to remove all VLAs from the kernel[1] this moves the sg_list
>> variable off the stack, as already done for other allocated buffers in
>> adpt_i2o_passthru(). Additionally consolidates the error path for kfree().
>
> Friendly ping! How does this look for v4.18?

Applied to 4.18/scsi-queue. Thanks!

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH v3 0/3] Fix UFS and devfreq interaction

[Martin K. Petersen]

Bjorn,

> With the introduction of f1d981eaecf8 ("PM / devfreq: Use the
> available min/max frequency") the UFS host controller driver (UFSHCD)
> stopped probing for platforms that supports frequency scaling,
> e.g. all modern Qualcomm platforms.

Applied to 4.18/scsi-queue. Thank you!

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH 1/1] scsi: storvsc: Avoid allocating memory for temp cpumasks

[Martin K. Petersen]

Michael,

> Current code allocates 240 Kbytes (in typical configs) for each
> synthetic SCSI controller to use as temp cpumask variables.  Recode to
> avoid needing the temp cpumask variables and remove the memory
> allocation.

Applied to 4.18/scsi-queue. Thanks!

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH 00/25] zfcp: updates for v4.18

[Martin K. Petersen]

Steffen,

> this is the zfcp patch set for the v4.18 merge window.  The patches
> apply to Martin's 4.18/scsi-queue.

Applied to 4.18/scsi-queue. Thank you!

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH 0/7] Miscellaneous patches and bug fixes

[Martin K. Petersen]

Uma,

> This patch series adds few improvements to the cxlflash driver and it
> also contains couple of bug fixes.

Applied to 4.18/scsi-queue, thank you!

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH 0/6] hisi_sas: improve DQ locking

[Martin K. Petersen]

John,

> This patchset introduces some patches to much improve DQ lockout for
> sending commands to the HW.

Applied to 4.18/scsi-queue. Thanks!

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH] scsi: dpt_i2o: Remove VLA usage

[Kees Cook]

On Wed, May 2, 2018 at 3:21 PM, Kees Cook  wrote:
> On the quest to remove all VLAs from the kernel[1] this moves the sg_list
> variable off the stack, as already done for other allocated buffers in
> adpt_i2o_passthru(). Additionally consolidates the error path for kfree().
>
> [1] 
> https://lkml.kernel.org/r/CA+55aFzCG-zNmZwX4A2FQpadafLfEzK6CC=qpxydaacu1rq...@mail.gmail.com
>
> Signed-off-by: Kees Cook 

Friendly ping! How does this look for v4.18?

Thanks!

-Kees

> ---
>  drivers/scsi/dpt_i2o.c | 21 ++---
>  1 file changed, 14 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/scsi/dpt_i2o.c b/drivers/scsi/dpt_i2o.c
> index 5ceea8da7bb6..37de8fb186d7 100644
> --- a/drivers/scsi/dpt_i2o.c
> +++ b/drivers/scsi/dpt_i2o.c
> @@ -1706,7 +1706,7 @@ static int adpt_i2o_passthru(adpt_hba* pHba, u32 __user 
> *arg)
> u32 reply_size = 0;
> u32 __user *user_msg = arg;
> u32 __user * user_reply = NULL;
> -   void *sg_list[pHba->sg_tablesize];
> +   void **sg_list = NULL;
> u32 sg_offset = 0;
> u32 sg_count = 0;
> int sg_index = 0;
> @@ -1748,19 +1748,23 @@ static int adpt_i2o_passthru(adpt_hba* pHba, u32 
> __user *arg)
> msg[2] = 0x4000; // IOCTL context
> msg[3] = adpt_ioctl_to_context(pHba, reply);
> if (msg[3] == (u32)-1) {
> -   kfree(reply);
> -   return -EBUSY;
> +   rcode = -EBUSY;
> +   goto free;
> }
>
> -   memset(sg_list,0, sizeof(sg_list[0])*pHba->sg_tablesize);
> +   sg_list = kcalloc(pHba->sg_tablesize, sizeof(*sg_list), GFP_KERNEL);
> +   if (!sg_list) {
> +   rcode = -ENOMEM;
> +   goto free;
> +   }
> if(sg_offset) {
> // TODO add 64 bit API
> struct sg_simple_element *sg =  (struct sg_simple_element*) 
> (msg+sg_offset);
> sg_count = (size - sg_offset*4) / sizeof(struct 
> sg_simple_element);
> if (sg_count > pHba->sg_tablesize){
> printk(KERN_DEBUG"%s:IOCTL SG List too large (%u)\n", 
> pHba->name,sg_count);
> -   kfree (reply);
> -   return -EINVAL;
> +   rcode = -EINVAL;
> +   goto free;
> }
>
> for(i = 0; i < sg_count; i++) {
> @@ -1879,7 +1883,6 @@ static int adpt_i2o_passthru(adpt_hba* pHba, u32 __user 
> *arg)
> if (rcode != -ETIME && rcode != -EINTR) {
> struct sg_simple_element *sg =
> (struct sg_simple_element*) (msg +sg_offset);
> -   kfree (reply);
> while(sg_index) {
> if(sg_list[--sg_index]) {
> dma_free_coherent(>pDev->dev,
> @@ -1889,6 +1892,10 @@ static int adpt_i2o_passthru(adpt_hba* pHba, u32 
> __user *arg)
> }
> }
> }
> +
> +free:
> +   kfree(sg_list);
> +   kfree(reply);
> return rcode;
>  }
>
> --
> 2.17.0
>
>
> --
> Kees Cook
> Pixel Security



-- 
Kees Cook
Pixel Security

Re: [PATCH 15/25] workqueue,zfcp: set description for port work items with their WWPN as context

[Tejun Heo]

On Thu, May 17, 2018 at 07:14:57PM +0200, Steffen Maier wrote:
> As a prerequisite, complement commit 3d1cb2059d93 ("workqueue: include
> workqueue info when printing debug dump of a worker task") to be usable
> with kernel modules by exporting the symbol set_worker_desc().
> Current built-in user was introduced with commit ef3b101925f2 ("writeback:
> set worker desc to identify writeback workers in task dumps").
> 
> Can help distinguishing work items which do not have adapter scope.
> Description is printed out with task dump for debugging on
> WARN, BUG, panic, or magic-sysrq [show-task-states(t)].

Acked-by: Tejun Heo 

Thanks.

-- 
tejun

Re: [PATCH 0/4] Add required changes to ufshcd to support exynos ufs hci

[Martin K. Petersen]

Alim,

> These patches are part of a larger patch series [1] which attempts
> upstreaming EXYNOS UFS driver support. There was not much activities
> after v5 of that series. In between I saw there were other teams in
> Samsung tried upstreaming the same, but that has not really gone
> anywhere.  I have taken this task again and here is another attempt to
> upstream exynos-ufs driver support. I have divided the patches into
> two series, one which adds required infra in the ufshcd core needed by
> exynos-ufs driver and other part will have actual exynos-ufs
> driver. Splitting this has a advantage of less reviewing over head.

Applied to 4.18/scsi-queue. Thank you!

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH] scsi: ufs: ufshcd: Remove VLA usage

[Martin K. Petersen]

Kees,

> On the quest to remove all VLAs from the kernel[1] this moves buffers
> off the stack. In the second instance, this collapses two separately
> allocated buffers into a single buffer, since they are used
> consecutively, which saves 256 bytes (QUERY_DESC_MAX_SIZE + 1) of
> stack space.

Applied to 4.18/scsi-queue. Thank you!

-- 
Martin K. Petersen  Oracle Linux Engineering

Re: [PATCH] scsi: sg: allocate with __GFP_ZERO in sg_build_indirect()

[Martin K. Petersen]

Alexander,

> This shall help avoid copying uninitialized memory to the userspace
> when calling ioctl(fd, SG_IO) with an empty command.

Applied to 4.17/scsi-fixes. Thank you!

-- 
Martin K. Petersen  Oracle Linux Engineering

[PATCH] scsi: sg: allocate with __GFP_ZERO in sg_build_indirect()

[Alexander Potapenko]

This shall help avoid copying uninitialized memory to the userspace
when calling ioctl(fd, SG_IO) with an empty command.

Reported-by: syzbot+7d26fc1eea198488d...@syzkaller.appspotmail.com
Cc: sta...@vger.kernel.org
Signed-off-by: Alexander Potapenko 
Acked-by: Douglas Gilbert 
Reviewed-by: Johannes Thumshirn 
---
 drivers/scsi/sg.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/scsi/sg.c b/drivers/scsi/sg.c
index c198b96368dd..5c40d809830f 100644
--- a/drivers/scsi/sg.c
+++ b/drivers/scsi/sg.c
@@ -1894,7 +1894,7 @@ sg_build_indirect(Sg_scatter_hold * schp, Sg_fd * sfp, 
int buff_size)
num = (rem_sz > scatter_elem_sz_prev) ?
scatter_elem_sz_prev : rem_sz;
 
-   schp->pages[k] = alloc_pages(gfp_mask, order);
+   schp->pages[k] = alloc_pages(gfp_mask | __GFP_ZERO, order);
if (!schp->pages[k])
goto out;
 
-- 
2.17.0.441.gb46fe60e1d-goog

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

[John Garry]

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?



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?

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;
 }

Re: [Drbd-dev] [PATCH 28/42] drbd: switch to proc_create_single

[Lars Ellenberg]

On Wed, May 16, 2018 at 11:43:32AM +0200, Christoph Hellwig wrote:
> And stop messing with try_module_get on THIS_MODULE, which doesn't make
> any sense here.

The idea was to increase module count on /proc/drbd access.

If someone holds /proc/drbd open, previously rmmod would
"succeed" in starting the unload, but then block on remove_proc_entry,
leading to a situation where the lsmod does not show drbd anymore,
but /proc/drbd being still there (but no longer accessible).

I'd rather have rmmod fail up front in this case.
And try_module_get() seemed most appropriate.

Lars

[Bug 199703] HPSA blocking boot on HP smart Array P400

[bugzilla-daemon]

https://bugzilla.kernel.org/show_bug.cgi?id=199703

--- Comment #6 from Roberto M. (roby_program...@fastwebnet.it) ---
Sorry I mean kernel 4.14.1 not 4.13.1

-- 
You are receiving this mail because:
You are the assignee for the bug.

Re: [PATCH] Bsg referencing parent device

[Christoph Hellwig]

The idea looks pretty reasonable, but once that is done we can get rid of
the ->release callback entirely and just handle it in the callers.
Something like the untested patch below:

diff --git a/block/bsg-lib.c b/block/bsg-lib.c
index fc2e5ff2c4b9..9419def8c017 100644
--- a/block/bsg-lib.c
+++ b/block/bsg-lib.c
@@ -303,11 +303,9 @@ static void bsg_exit_rq(struct request_queue *q, struct 
request *req)
  * @name: device to give bsg device
  * @job_fn: bsg job handler
  * @dd_job_size: size of LLD data needed for each job
- * @release: @dev release function
  */
 struct request_queue *bsg_setup_queue(struct device *dev, const char *name,
-   bsg_job_fn *job_fn, int dd_job_size,
-   void (*release)(struct device *))
+   bsg_job_fn *job_fn, int dd_job_size)
 {
struct request_queue *q;
int ret;
@@ -331,7 +329,7 @@ struct request_queue *bsg_setup_queue(struct device *dev, 
const char *name,
blk_queue_softirq_done(q, bsg_softirq_done);
blk_queue_rq_timeout(q, BLK_DEFAULT_SG_TIMEOUT);
 
-   ret = bsg_register_queue(q, dev, name, _transport_ops, release);
+   ret = bsg_register_queue(q, dev, name, _transport_ops);
if (ret) {
printk(KERN_ERR "%s: bsg interface failed to "
   "initialize - register queue\n", dev->kobj.name);
diff --git a/block/bsg.c b/block/bsg.c
index defa06c11858..fe1e5632e5d1 100644
--- a/block/bsg.c
+++ b/block/bsg.c
@@ -650,18 +650,6 @@ static struct bsg_device *bsg_alloc_device(void)
return bd;
 }
 
-static void bsg_kref_release_function(struct kref *kref)
-{
-   struct bsg_class_device *bcd =
-   container_of(kref, struct bsg_class_device, ref);
-   struct device *parent = bcd->parent;
-
-   if (bcd->release)
-   bcd->release(bcd->parent);
-
-   put_device(parent);
-}
-
 static int bsg_put_device(struct bsg_device *bd)
 {
int ret = 0, do_free;
@@ -694,7 +682,6 @@ static int bsg_put_device(struct bsg_device *bd)
 
kfree(bd);
 out:
-   kref_put(>bsg_dev.ref, bsg_kref_release_function);
if (do_free)
blk_put_queue(q);
return ret;
@@ -760,8 +747,6 @@ static struct bsg_device *bsg_get_device(struct inode 
*inode, struct file *file)
 */
mutex_lock(_mutex);
bcd = idr_find(_minor_idr, iminor(inode));
-   if (bcd)
-   kref_get(>ref);
mutex_unlock(_mutex);
 
if (!bcd)
@@ -772,8 +757,6 @@ static struct bsg_device *bsg_get_device(struct inode 
*inode, struct file *file)
return bd;
 
bd = bsg_add_device(inode, bcd->queue, file);
-   if (IS_ERR(bd))
-   kref_put(>ref, bsg_kref_release_function);
 
return bd;
 }
@@ -913,25 +896,17 @@ void bsg_unregister_queue(struct request_queue *q)
sysfs_remove_link(>kobj, "bsg");
device_unregister(bcd->class_dev);
bcd->class_dev = NULL;
-   kref_put(>ref, bsg_kref_release_function);
mutex_unlock(_mutex);
 }
 EXPORT_SYMBOL_GPL(bsg_unregister_queue);
 
 int bsg_register_queue(struct request_queue *q, struct device *parent,
-   const char *name, const struct bsg_ops *ops,
-   void (*release)(struct device *))
+   const char *name, const struct bsg_ops *ops)
 {
struct bsg_class_device *bcd;
dev_t dev;
int ret;
struct device *class_dev = NULL;
-   const char *devname;
-
-   if (name)
-   devname = name;
-   else
-   devname = dev_name(parent);
 
/*
 * we need a proper transport to send commands, not a stacked device
@@ -955,15 +930,12 @@ int bsg_register_queue(struct request_queue *q, struct 
device *parent,
 
bcd->minor = ret;
bcd->queue = q;
-   bcd->parent = get_device(parent);
-   bcd->release = release;
bcd->ops = ops;
-   kref_init(>ref);
dev = MKDEV(bsg_major, bcd->minor);
-   class_dev = device_create(bsg_class, parent, dev, NULL, "%s", devname);
+   class_dev = device_create(bsg_class, parent, dev, NULL, "%s", name);
if (IS_ERR(class_dev)) {
ret = PTR_ERR(class_dev);
-   goto put_dev;
+   goto idr_remove;
}
bcd->class_dev = class_dev;
 
@@ -978,8 +950,7 @@ int bsg_register_queue(struct request_queue *q, struct 
device *parent,
 
 unregister_class_dev:
device_unregister(class_dev);
-put_dev:
-   put_device(parent);
+idr_remove:
idr_remove(_minor_idr, bcd->minor);
 unlock:
mutex_unlock(_mutex);
@@ -993,7 +964,7 @@ int bsg_scsi_register_queue(struct request_queue *q, struct 
device *parent)
return -EINVAL;
}
 
-   return bsg_register_queue(q, parent, NULL, _scsi_ops, NULL);
+   return bsg_register_queue(q, parent, dev_name(parent), _scsi_ops);
 }
 EXPORT_SYMBOL_GPL(bsg_scsi_register_queue);
 
diff --git 

Re: [PATCH 38/42] isdn: replace ->proc_fops with ->proc_show

[Christoph Hellwig]

On Fri, May 18, 2018 at 10:43:46AM +0200, Paul Bolle wrote:
> > iif->ctr.release_appl  = gigaset_release_appl;
> > iif->ctr.send_message  = gigaset_send_message;
> > -   iif->ctr.procinfo  = gigaset_procinfo;
> 
> Is this intentional? You didn't touch the procinfo method in the other ISDN
> drivers, as far as I can see.
> 
> (If it was intentional, gigaset_procinfo() can of course be removed.)

Already fixed in the branch in Als tree.

Re: [PATCH 38/42] isdn: replace ->proc_fops with ->proc_show

[Paul Bolle]

Hi Christoph,

(I don't think the patches of this series ever hit the ISDN related addresses
still found in MAINTAINERS. And now I might be a bit late.) 

Christoph Hellwig schreef op wo 16-05-2018 om 11:43 [+0200]:
> diff --git a/drivers/isdn/gigaset/capi.c b/drivers/isdn/gigaset/capi.c
> index ccec7778cad2..dac5cd35e901 100644
> --- a/drivers/isdn/gigaset/capi.c
> +++ b/drivers/isdn/gigaset/capi.c
> @@ -2437,19 +2437,6 @@ static int gigaset_proc_show(struct seq_file *m, void 
> *v)
>   return 0;
>  }
>  
> -static int gigaset_proc_open(struct inode *inode, struct file *file)
> -{
> - return single_open(file, gigaset_proc_show, PDE_DATA(inode));
> -}
> -
> -static const struct file_operations gigaset_proc_fops = {
> - .owner  = THIS_MODULE,
> - .open   = gigaset_proc_open,
> - .read   = seq_read,
> - .llseek = seq_lseek,
> - .release= single_release,
> -};
> -
>  /**
>   * gigaset_isdn_regdev() - register device to LL
>   * @cs:  device descriptor structure.
> @@ -2478,8 +2465,7 @@ int gigaset_isdn_regdev(struct cardstate *cs, const 
> char *isdnid)
>   iif->ctr.register_appl = gigaset_register_appl;
>   iif->ctr.release_appl  = gigaset_release_appl;
>   iif->ctr.send_message  = gigaset_send_message;
> - iif->ctr.procinfo  = gigaset_procinfo;

Is this intentional? You didn't touch the procinfo method in the other ISDN
drivers, as far as I can see.

(If it was intentional, gigaset_procinfo() can of course be removed.)

> - iif->ctr.proc_fops = _proc_fops;
> + iif->ctr.proc_show = gigaset_proc_show,
>   INIT_LIST_HEAD(>appls);
>   skb_queue_head_init(>sendqueue);
>   atomic_set(>sendqlen, 0);

Thanks,


Paul Bolle

[PATCH v3 1/3] scsi: ufs: Extract devfreq registration

[Bjorn Andersson]

Failing to register with devfreq leaves hba->devfreq assigned, which
causes the error path to dereference the ERR_PTR(). Rather than bolting
on more conditionals, move the call of devm_devfreq_add_device() into
it's own function and only update hba->devfreq once it's successfully
registered.

The subsequent patch builds upon this to make UFS actually work again,
as it's been broken since f1d981eaecf8 ("PM / devfreq: Use the available
min/max frequency")

Also switch to use DEVFREQ_GOV_SIMPLE_ONDEMAND constant.

Reviewed-by: Chanwoo Choi 
Signed-off-by: Bjorn Andersson 
---

Changes since v2:
- Use DEVFREQ_GOV_SIMPLE_ONDEMAND, per Chanwoo's recommendation
- Picked up Chanwoo's R-b

Changes since v1:
- None

 drivers/scsi/ufs/ufshcd.c | 31 ++-
 1 file changed, 22 insertions(+), 9 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index 00e79057f870..f04902a066cb 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1287,6 +1287,26 @@ static struct devfreq_dev_profile ufs_devfreq_profile = {
.get_dev_status = ufshcd_devfreq_get_dev_status,
 };
 
+static int ufshcd_devfreq_init(struct ufs_hba *hba)
+{
+   struct devfreq *devfreq;
+   int ret;
+
+   devfreq = devm_devfreq_add_device(hba->dev,
+   _devfreq_profile,
+   DEVFREQ_GOV_SIMPLE_ONDEMAND,
+   NULL);
+   if (IS_ERR(devfreq)) {
+   ret = PTR_ERR(devfreq);
+   dev_err(hba->dev, "Unable to register with devfreq %d\n", ret);
+   return ret;
+   }
+
+   hba->devfreq = devfreq;
+
+   return 0;
+}
+
 static void __ufshcd_suspend_clkscaling(struct ufs_hba *hba)
 {
unsigned long flags;
@@ -6439,16 +6459,9 @@ static int ufshcd_probe_hba(struct ufs_hba *hba)
sizeof(struct ufs_pa_layer_attr));
hba->clk_scaling.saved_pwr_info.is_valid = true;
if (!hba->devfreq) {
-   hba->devfreq = devm_devfreq_add_device(hba->dev,
-   _devfreq_profile,
-   "simple_ondemand",
-   NULL);
-   if (IS_ERR(hba->devfreq)) {
-   ret = PTR_ERR(hba->devfreq);
-   dev_err(hba->dev, "Unable to register 
with devfreq %d\n",
-   ret);
+   ret = ufshcd_devfreq_init(hba);
+   if (ret)
goto out;
-   }
}
hba->clk_scaling.is_allowed = true;
}
-- 
2.17.0

[PATCH v3 0/3] Fix UFS and devfreq interaction

[Bjorn Andersson]

With the introduction of f1d981eaecf8 ("PM / devfreq: Use the available min/max
frequency") the UFS host controller driver (UFSHCD) stopped probing for
platforms that supports frequency scaling, e.g. all modern Qualcomm platforms.

The cause of this was UFSHCD's reliance of not registering any frequencies and
then being called by devfreq to switch between the frequencies 0 and UINT_MAX.

The devfreq code implies that the client is able to pass the frequency table,
instead of relying on opp tables, but as concluded after v1 this is not
compliant with devfreq cooling, which will enable and disable opp entries in
order to limit the valid frequencies. So instead the UFSHCD driver is modified
to read the freq-table and register the first clock's two rates as the two
available opp levels.


This follows the first patch which facilitates the implementation of this in a
clean fashion, and removes the kernel panic which previously happened when
devfreq initialization failed.


With this UFS is once again functional on the db820c, and is needed to get UFS
working on SDM845 (both tested).

Added in v3 is the dts patch for Andy to introduce UFS in msm8996 and db820c,
now that it finally works again.

Bjorn Andersson (3):
  scsi: ufs: Extract devfreq registration
  scsi: ufs: Use freq table with devfreq
  arm64: dts: qcom: msm8996: Add ufs related nodes

 arch/arm64/boot/dts/qcom/apq8096-db820c.dtsi |  8 ++
 arch/arm64/boot/dts/qcom/msm8996.dtsi| 85 
 drivers/scsi/ufs/ufshcd.c| 76 +
 3 files changed, 154 insertions(+), 15 deletions(-)

-- 
2.17.0

[PATCH v3 2/3] scsi: ufs: Use freq table with devfreq

[Bjorn Andersson]

devfreq requires that the client operates on actual frequencies, not
only 0 and UMAX_INT and as such UFS brok with the introduction of
f1d981eaecf8 ("PM / devfreq: Use the available min/max frequency").

This patch registers the frequencies of the first clock with devfreq and
use these to determine if we're trying to step up or down.

Reviewed-by: Chanwoo Choi  [for devfreq & OPP part]
Reviewed-by: Subhash Jadavani 
Signed-off-by: Bjorn Andersson 
---

Changes since v2:
- Picked up R-b from Chanwoo and Subhash

Chances since v1:
- Register min_freq and max_freq as opp levels.
- Unregister opp levels on removal, to make e.g. probe defer working

 drivers/scsi/ufs/ufshcd.c | 47 +--
 1 file changed, 40 insertions(+), 7 deletions(-)

diff --git a/drivers/scsi/ufs/ufshcd.c b/drivers/scsi/ufs/ufshcd.c
index f04902a066cb..3d46a70ed41d 100644
--- a/drivers/scsi/ufs/ufshcd.c
+++ b/drivers/scsi/ufs/ufshcd.c
@@ -1200,16 +1200,13 @@ static int ufshcd_devfreq_target(struct device *dev,
struct ufs_hba *hba = dev_get_drvdata(dev);
ktime_t start;
bool scale_up, sched_clk_scaling_suspend_work = false;
+   struct list_head *clk_list = >clk_list_head;
+   struct ufs_clk_info *clki;
unsigned long irq_flags;
 
if (!ufshcd_is_clkscaling_supported(hba))
return -EINVAL;
 
-   if ((*freq > 0) && (*freq < UINT_MAX)) {
-   dev_err(hba->dev, "%s: invalid freq = %lu\n", __func__, *freq);
-   return -EINVAL;
-   }
-
spin_lock_irqsave(hba->host->host_lock, irq_flags);
if (ufshcd_eh_in_progress(hba)) {
spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
@@ -1219,7 +1216,13 @@ static int ufshcd_devfreq_target(struct device *dev,
if (!hba->clk_scaling.active_reqs)
sched_clk_scaling_suspend_work = true;
 
-   scale_up = (*freq == UINT_MAX) ? true : false;
+   if (list_empty(clk_list)) {
+   spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
+   goto out;
+   }
+
+   clki = list_first_entry(>clk_list_head, struct ufs_clk_info, list);
+   scale_up = (*freq == clki->max_freq) ? true : false;
if (!ufshcd_is_devfreq_scaling_required(hba, scale_up)) {
spin_unlock_irqrestore(hba->host->host_lock, irq_flags);
ret = 0;
@@ -1289,16 +1292,29 @@ static struct devfreq_dev_profile ufs_devfreq_profile = 
{
 
 static int ufshcd_devfreq_init(struct ufs_hba *hba)
 {
+   struct list_head *clk_list = >clk_list_head;
+   struct ufs_clk_info *clki;
struct devfreq *devfreq;
int ret;
 
-   devfreq = devm_devfreq_add_device(hba->dev,
+   /* Skip devfreq if we don't have any clocks in the list */
+   if (list_empty(clk_list))
+   return 0;
+
+   clki = list_first_entry(clk_list, struct ufs_clk_info, list);
+   dev_pm_opp_add(hba->dev, clki->min_freq, 0);
+   dev_pm_opp_add(hba->dev, clki->max_freq, 0);
+
+   devfreq = devfreq_add_device(hba->dev,
_devfreq_profile,
DEVFREQ_GOV_SIMPLE_ONDEMAND,
NULL);
if (IS_ERR(devfreq)) {
ret = PTR_ERR(devfreq);
dev_err(hba->dev, "Unable to register with devfreq %d\n", ret);
+
+   dev_pm_opp_remove(hba->dev, clki->min_freq);
+   dev_pm_opp_remove(hba->dev, clki->max_freq);
return ret;
}
 
@@ -1307,6 +1323,22 @@ static int ufshcd_devfreq_init(struct ufs_hba *hba)
return 0;
 }
 
+static void ufshcd_devfreq_remove(struct ufs_hba *hba)
+{
+   struct list_head *clk_list = >clk_list_head;
+   struct ufs_clk_info *clki;
+
+   if (!hba->devfreq)
+   return;
+
+   devfreq_remove_device(hba->devfreq);
+   hba->devfreq = NULL;
+
+   clki = list_first_entry(clk_list, struct ufs_clk_info, list);
+   dev_pm_opp_remove(hba->dev, clki->min_freq);
+   dev_pm_opp_remove(hba->dev, clki->max_freq);
+}
+
 static void __ufshcd_suspend_clkscaling(struct ufs_hba *hba)
 {
unsigned long flags;
@@ -7005,6 +7037,7 @@ static void ufshcd_hba_exit(struct ufs_hba *hba)
if (hba->devfreq)
ufshcd_suspend_clkscaling(hba);
destroy_workqueue(hba->clk_scaling.workq);
+   ufshcd_devfreq_remove(hba);
}
ufshcd_setup_clocks(hba, false);
ufshcd_setup_hba_vreg(hba, false);
-- 
2.17.0