[Patch] cleanup: GFP_ATOMIC to GFP_KERNEL in scsi_scan.c

[Patch] cleanup: GFP_ATOMIC to GFP_KERNEL in scsi_scan.c

[Martin Peschke]

It seems to be safe to replace all 4 occurrences of GFP_ATOMIC in
scsi_scan.c by GFP_KERNEL. I found that calling code always held a mutex
(indicating process context) while not acquiring a spin_lock or such
inside the mutex sections and when using GFP_ATOMIC (see details below).

http://marc.theaimsgroup.com/?l=linux-scsi&m=115522223917634&w=2
indicates that these occurrences of GFP_ATOMIC are just there for
historical reasons.

Patch ist against 2.6.18-rc4-mm2. Please consider applying this cleanup.
It didn't break my test machine with 3 SCSI disks attached.

kzalloc(..., GFP_ATOMIC) in scsi_alloc_sdev()
        scsi_get_host_dev()
                => holds mutex
        scsi_report_lun_scan()
                __scsi_scan_target()
                        scsi_scan_target()
                                => holds mutex
                        scsi_scan_channel()
                                scsi_scan_host_selected()
                                        => holds mutex
        scsi_probe_and_add_lun()
                scsi_sequential_lun_scan()
                        __scsi_scan_target()
                                ... => holds mutex (see above)
                scsi_report_lun_scan()
                        ... => holds mutex (see above)
                __scsi_add_device()
                        => holds mutex
                __scsi_scan_target()
                        ... => holds mutex (see above)

kmalloc(..., GFP_ATOMIC) in scsi_add_lun()
        scsi_probe_and_add_lun()
                ... => holds mutex (see above)

kmalloc(..., GFP_ATOMIC) in scsi_probe_and_add_lun()
        ... => holds mutex (see above)

kmalloc(..., GFP_ATOMIC) in scsi_report_lun_scan()
        ... => holds mutex (see above)

Signed-off-by: Martin Peschke <mp3@de.ibm.com>
---

 scsi_scan.c |    8 ++++----
 1 files changed, 4 insertions(+), 4 deletions(-)

diff -ur a/drivers/scsi/scsi_scan.c b/drivers/scsi/scsi_scan.c
--- a/drivers/scsi/scsi_scan.c	2006-08-21 20:10:26.000000000 +0200
+++ b/drivers/scsi/scsi_scan.c	2006-08-21 20:07:57.000000000 +0200
@@ -205,7 +205,7 @@
 	struct Scsi_Host *shost = dev_to_shost(starget->dev.parent);
 
 	sdev = kzalloc(sizeof(*sdev) + shost->transportt->device_size,
-		       GFP_ATOMIC);
+		       GFP_KERNEL);
 	if (!sdev)
 		goto out;
 
@@ -640,7 +640,7 @@
 	 * scanning run at their own risk, or supply a user level program
 	 * that can correctly scan.
 	 */
-	sdev->inquiry = kmalloc(sdev->inquiry_len, GFP_ATOMIC);
+	sdev->inquiry = kmalloc(sdev->inquiry_len, GFP_KERNEL);
 	if (sdev->inquiry == NULL) {
 		return SCSI_SCAN_NO_RESPONSE;
 	}
@@ -898,7 +898,7 @@
 	if (!sdev)
 		goto out;
 
-	result = kmalloc(result_len, GFP_ATOMIC |
+	result = kmalloc(result_len, GFP_KERNEL |
 			((shost->unchecked_isa_dma) ? __GFP_DMA : 0));
 	if (!result)
 		goto out_free_sdev;
@@ -1194,7 +1194,7 @@
 	 * prevent us from finding any LUNs on this target.
 	 */
 	length = (max_scsi_report_luns + 1) * sizeof(struct scsi_lun);
-	lun_data = kmalloc(length, GFP_ATOMIC |
+	lun_data = kmalloc(length, GFP_KERNEL |
 			   (sdev->host->unchecked_isa_dma ? __GFP_DMA : 0));
 	if (!lun_data) {
 		printk(ALLOC_FAILURE_MSG, __FUNCTION__);

Re: [Patch] cleanup: GFP_ATOMIC to GFP_KERNEL in scsi_scan.c

[Stefan Richter]

Martin Peschke wrote:
> It seems to be safe to replace all 4 occurrences of GFP_ATOMIC in
> scsi_scan.c by GFP_KERNEL. I found that calling code always held a mutex
> (indicating process context) while not acquiring a spin_lock or such
> inside the mutex sections and when using GFP_ATOMIC (see details below).

Please use diff's -p option for postings like this.

Did you check Documentation/scsi/scsi_mid_low_api.txt with respect to
the detailed description of all exported functions which you modify? All
of them should contain a remark like "Might block: yes" or something
else in the way of "do not call in atomic context". Although I suppose
that all or most of them do so already.

If scsi_mid_low_api.txt does not fully reflect what your patch imposes,
please modify scsi_mid_low_api.txt in the same patch.

> http://marc.theaimsgroup.com/?l=linux-scsi&m=115522223917634&w=2
> indicates that these occurrences of GFP_ATOMIC are just there for
> historical reasons.
> 
> Patch ist against 2.6.18-rc4-mm2. Please consider applying this cleanup.
> It didn't break my test machine with 3 SCSI disks attached.
[...]

You need to make sure that it does not break _any_ caller. (SCSI is more
than the bundle of interconnect drivers for SPI hardware.) Also take
precautions for future callers or future changes to current callers.
-- 
Stefan Richter
-=====-=-==- =--- =-==-
http://arcgraph.de/sr/

Re: [Patch] cleanup: GFP_ATOMIC to GFP_KERNEL in scsi_scan.c

[Martin Peschke]

Stefan Richter wrote:
> Martin Peschke wrote:
>> It seems to be safe to replace all 4 occurrences of GFP_ATOMIC in
>> scsi_scan.c by GFP_KERNEL. I found that calling code always held a mutex
>> (indicating process context) while not acquiring a spin_lock or such
>> inside the mutex sections and when using GFP_ATOMIC (see details below).
> 
> Please use diff's -p option for postings like this.

okay

> Did you check Documentation/scsi/scsi_mid_low_api.txt with respect to
> the detailed description of all exported functions which you modify? All
> of them should contain a remark like "Might block: yes" or something
> else in the way of "do not call in atomic context". Although I suppose
> that all or most of them do so already.
> 
> If scsi_mid_low_api.txt does not fully reflect what your patch imposes,
> please modify scsi_mid_low_api.txt in the same patch.

Thanks for the hint.

My changes conform to this description, as far as scsi_mid_low_api.txt
covers the interfaces touched by my patch.

Looks like a documentation update is needed regardless of my patch:

scsi_get_host_dev(), scsi_scan_target(), __scsi_add_device() are not
documented, though being exported. These are the ones affected by my
patch. I didn't check for other misses.

scsi_mid_low_api.txt says scsi_add_host() never blocks. This function
calls sysfs routines which might block (on mutex), and it uses
GFP_KERNEL - the latter not being my fault :)

scsi_host_get() "currently may block but may be changed to not block" -
current code won't block.

(My observations come from 2.6.18-rc4-mm2.)

> You need to make sure that it does not break _any_ caller. (SCSI is more
> than the bundle of interconnect drivers for SPI hardware.) Also take
> precautions for future callers or future changes to current callers.

Sure. I found that all these interface functions acquire a mutex. That is,
any caller which doesn't guarantee process context would be broken anyway,
even without me changing GFP_ATOMICs.

Martin

Re: [Patch] cleanup: GFP_ATOMIC to GFP_KERNEL in scsi_scan.c

[Stefan Richter]

Martin Peschke wrote:
> My changes conform to this description, as far as scsi_mid_low_api.txt
> covers the interfaces touched by my patch.
> 
> Looks like a documentation update is needed regardless of my patch:
[...scsi_get_host_dev, scsi_scan_target, __scsi_add_device, 
scsi_add_host, scsi_host_get,...]

Thanks a lot for taking the time to clarify this.
-- 
Stefan Richter
-=====-=-==- =--- =-==-
http://arcgraph.de/sr/