[PATCH v2] sr: don't use GFP_DMA

[PATCH v2] sr: don't use GFP_DMA

[Christoph Hellwig]

The allocated buffers are used as a command payload, for which the block
layer and/or DMA API do the proper bounce buffering if needed.

Reported-by: Baoquan He <bhe@redhat.com>
Signed-off-by: Christoph Hellwig <hch@lst.de>
---

Changes since v1:
 - also cover the two callsites in sr_vendor.c

Diffstat:
 drivers/scsi/sr.c        | 2 +-
 drivers/scsi/sr_vendor.c | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
index 14c122839c409..f925b1f1f9ada 100644
--- a/drivers/scsi/sr.c
+++ b/drivers/scsi/sr.c
@@ -855,7 +855,7 @@ static void get_capabilities(struct scsi_cd *cd)
 
 
 	/* allocate transfer buffer */
-	buffer = kmalloc(512, GFP_KERNEL | GFP_DMA);
+	buffer = kmalloc(512, GFP_KERNEL);
 	if (!buffer) {
 		sr_printk(KERN_ERR, cd, "out of memory.\n");
 		return;
diff --git a/drivers/scsi/sr_vendor.c b/drivers/scsi/sr_vendor.c
index 1f988a1b9166f..a61635326ae0a 100644
--- a/drivers/scsi/sr_vendor.c
+++ b/drivers/scsi/sr_vendor.c
@@ -131,7 +131,7 @@ int sr_set_blocklength(Scsi_CD *cd, int blocklength)
 	if (cd->vendor == VENDOR_TOSHIBA)
 		density = (blocklength > 2048) ? 0x81 : 0x83;
 
-	buffer = kmalloc(512, GFP_KERNEL | GFP_DMA);
+	buffer = kmalloc(512, GFP_KERNEL);
 	if (!buffer)
 		return -ENOMEM;
 
@@ -179,7 +179,7 @@ int sr_cd_check(struct cdrom_device_info *cdi)
 	if (cd->cdi.mask & CDC_MULTI_SESSION)
 		return 0;
 
-	buffer = kmalloc(512, GFP_KERNEL | GFP_DMA);
+	buffer = kmalloc(512, GFP_KERNEL);
 	if (!buffer)
 		return -ENOMEM;
 
-- 
2.30.2

Re: [PATCH v2] sr: don't use GFP_DMA

[Baoquan He]

Hi Christoph,

On 12/22/21 at 10:08am, Christoph Hellwig wrote:
> The allocated buffers are used as a command payload, for which the block
> layer and/or DMA API do the proper bounce buffering if needed.
> 
> Reported-by: Baoquan He <bhe@redhat.com>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> 
> Changes since v1:
>  - also cover the two callsites in sr_vendor.c
> 
> Diffstat:
>  drivers/scsi/sr.c        | 2 +-
>  drivers/scsi/sr_vendor.c | 4 ++--
>  2 files changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/scsi/sr.c b/drivers/scsi/sr.c
> index 14c122839c409..f925b1f1f9ada 100644
> --- a/drivers/scsi/sr.c
> +++ b/drivers/scsi/sr.c
> @@ -855,7 +855,7 @@ static void get_capabilities(struct scsi_cd *cd)
>  
>  
>  	/* allocate transfer buffer */
> -	buffer = kmalloc(512, GFP_KERNEL | GFP_DMA);
> +	buffer = kmalloc(512, GFP_KERNEL);

Thanks a lot for doing this. When I browsed the code path, I come to
blk_rq_map_kern() but I am not sure if blk_queue_may_bounce() is true in
the sr_probe() case, then it may enter into bio_map_kern().

Next I will post my original patchset to mute the allocation failure if
it's requesting page from DMA zone and DMA zone has no managed page. And
meanwhile, I will try to collect those places of kmalloc(GFP_DMA) into a
RFC mail, see if we can change them one by one. Anyone can pick one
place to fix if interested or knowing it well. Finally, we can remove the
need of dma-kmalloc() as people suggested. Any comment?

For this patch, it's an awesome start, thx.

Reviewed-by: Baoquan He <bhe@redhat.com>


>  	if (!buffer) {
>  		sr_printk(KERN_ERR, cd, "out of memory.\n");
>  		return;
> diff --git a/drivers/scsi/sr_vendor.c b/drivers/scsi/sr_vendor.c
> index 1f988a1b9166f..a61635326ae0a 100644
> --- a/drivers/scsi/sr_vendor.c
> +++ b/drivers/scsi/sr_vendor.c
> @@ -131,7 +131,7 @@ int sr_set_blocklength(Scsi_CD *cd, int blocklength)
>  	if (cd->vendor == VENDOR_TOSHIBA)
>  		density = (blocklength > 2048) ? 0x81 : 0x83;
>  
> -	buffer = kmalloc(512, GFP_KERNEL | GFP_DMA);
> +	buffer = kmalloc(512, GFP_KERNEL);
>  	if (!buffer)
>  		return -ENOMEM;
>  
> @@ -179,7 +179,7 @@ int sr_cd_check(struct cdrom_device_info *cdi)
>  	if (cd->cdi.mask & CDC_MULTI_SESSION)
>  		return 0;
>  
> -	buffer = kmalloc(512, GFP_KERNEL | GFP_DMA);
> +	buffer = kmalloc(512, GFP_KERNEL);
>  	if (!buffer)
>  		return -ENOMEM;
>  
> -- 
> 2.30.2
> 

Re: [PATCH v2] sr: don't use GFP_DMA

[Christoph Hellwig]

On Wed, Dec 22, 2021 at 05:37:07PM +0800, Baoquan He wrote:
> >  	/* allocate transfer buffer */
> > -	buffer = kmalloc(512, GFP_KERNEL | GFP_DMA);
> > +	buffer = kmalloc(512, GFP_KERNEL);
> 
> Thanks a lot for doing this. When I browsed the code path, I come to
> blk_rq_map_kern() but I am not sure if blk_queue_may_bounce() is true in
> the sr_probe() case, then it may enter into bio_map_kern().

blk_queue_may_bounce will be entered for the few drives that set
BLK_BOUNCE_HIGH because they can't handle highmem (which is a subset
of the non-ZONE_DMA memory).  The only driver that actually requires
ZONE_DMA based bouncing is ps3rom, and that driver does a manual
and ubconditional bounce buffering.

> Next I will post my original patchset to mute the allocation failure if
> it's requesting page from DMA zone and DMA zone has no managed page. And
> meanwhile, I will try to collect those places of kmalloc(GFP_DMA) into a
> RFC mail, see if we can change them one by one. Anyone can pick one
> place to fix if interested or knowing it well.

I've already sent out patches for all of drivers/scsi/ today, except
for the ps3rom bounce buffer alllocation which is fine but should
probably be changed to use the page allocator directly.

> Finally, we can remove the
> need of dma-kmalloc() as people suggested. Any comment?

Yes.

Re: [PATCH v2] sr: don't use GFP_DMA

[Baoquan He]

On 12/22/21 at 10:42am, Christoph Hellwig wrote:
> On Wed, Dec 22, 2021 at 05:37:07PM +0800, Baoquan He wrote:
> > >  	/* allocate transfer buffer */
> > > -	buffer = kmalloc(512, GFP_KERNEL | GFP_DMA);
> > > +	buffer = kmalloc(512, GFP_KERNEL);
> > 
> > Thanks a lot for doing this. When I browsed the code path, I come to
> > blk_rq_map_kern() but I am not sure if blk_queue_may_bounce() is true in
> > the sr_probe() case, then it may enter into bio_map_kern().
> 
> blk_queue_may_bounce will be entered for the few drives that set
> BLK_BOUNCE_HIGH because they can't handle highmem (which is a subset
> of the non-ZONE_DMA memory).  The only driver that actually requires
> ZONE_DMA based bouncing is ps3rom, and that driver does a manual
> and ubconditional bounce buffering.

Got it now, thx.

> 
> > Next I will post my original patchset to mute the allocation failure if
> > it's requesting page from DMA zone and DMA zone has no managed page. And
> > meanwhile, I will try to collect those places of kmalloc(GFP_DMA) into a
> > RFC mail, see if we can change them one by one. Anyone can pick one
> > place to fix if interested or knowing it well.
> 
> I've already sent out patches for all of drivers/scsi/ today, except
> for the ps3rom bounce buffer alllocation which is fine but should
> probably be changed to use the page allocator directly.

Sounds really great. 

Any thought or plan for those callsites in other places? Possibly we can
skip those s390 related drivers since s390 only has DMA zone, no DMA32,
it should be OK.

And could you please also add me to CC when send out these patches? We
have this problem in our RHEL8 which is based on kernel4.18, if finally
removing dma-kmalloc(), we need back port these driver fixes too. If
not paying attention, these patches may scatter in different
sub-components and unnoticable.

> 
> > Finally, we can remove the
> > need of dma-kmalloc() as people suggested. Any comment?
> 
> Yes.
> 

Re: [PATCH v2] sr: don't use GFP_DMA

[Martin K. Petersen]

Christoph,

> The allocated buffers are used as a command payload, for which the block
> layer and/or DMA API do the proper bounce buffering if needed.

Applied to 5.17/scsi-staging, thanks!

-- 
Martin K. Petersen	Oracle Linux Engineering

Re: [PATCH v2] sr: don't use GFP_DMA

[Christoph Hellwig]

On Wed, Dec 22, 2021 at 06:40:46PM +0800, Baoquan He wrote:
> Any thought or plan for those callsites in other places? Possibly we can
> skip those s390 related drivers since s390 only has DMA zone, no DMA32,
> it should be OK.

Yes, this needs a bit of an audit.  A lot of them might be best handled
by the subsysem maintainers, e.g. for crypto media and sound.

> And could you please also add me to CC when send out these patches? We
> have this problem in our RHEL8 which is based on kernel4.18, if finally
> removing dma-kmalloc(), we need back port these driver fixes too. If
> not paying attention, these patches may scatter in different
> sub-components and unnoticable.

I already sent them out yesterday.  Just look for everything with
GFP_DMA in the subject line in the current scsi tree for 5.17.

Re: [PATCH v2] sr: don't use GFP_DMA

[Baoquan He]

On 12/23/21 at 10:01am, Christoph Hellwig wrote:
> On Wed, Dec 22, 2021 at 06:40:46PM +0800, Baoquan He wrote:
> > Any thought or plan for those callsites in other places? Possibly we can
> > skip those s390 related drivers since s390 only has DMA zone, no DMA32,
> > it should be OK.
> 
> Yes, this needs a bit of an audit.  A lot of them might be best handled
> by the subsysem maintainers, e.g. for crypto media and sound.

Yes, agree. I can send a mail to subsystem maintainers about this, ask
them for help.

> 
> > And could you please also add me to CC when send out these patches? We
> > have this problem in our RHEL8 which is based on kernel4.18, if finally
> > removing dma-kmalloc(), we need back port these driver fixes too. If
> > not paying attention, these patches may scatter in different
> > sub-components and unnoticable.
> 
> I already sent them out yesterday.  Just look for everything with
> GFP_DMA in the subject line in the current scsi tree for 5.17.

Got, thx.