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

[Baoquan He <bhe@redhat.com>]

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
>