Re: [PATCH] HID: intel-ish-hid: Use dma_alloc_coherent for firmware update

[srinivas pandruvada <srinivas.pandruvada@linux.intel.com>]

On Tue, 2022-02-08 at 21:09 -0800, Gwendal Grignou wrote:
> Allocating memory with kmalloc and GPF_DMA32 is not allowed, the
> allocator will ignore the attribute.
> 
Looks like there is new error flow added here for this flag.
Is this just removing GFP_DMA32 not enough?

> Instead, use dma_alloc_coherent() API as we allocate a small amount
> of
> memory to transfer firmware fragment to the ISH.
> 
> On Arcada chromebook, after the patch the warning:
> "Unexpected gfp: 0x4 (GFP_DMA32). Fixing up to gfp: 0xcc0
> (GFP_KERNEL).  Fix your code!"
> is gone. The ISH firmware is loaded properly and we can interact with
> the ISH:
> > ectool  --name cros_ish version
> ...
> Build info:    arcada_ish_v2.0.3661+3c1a1c1ae0 2022-02-08 05:37:47
> @localhost
> Tool version:  v2.0.12300-900b03ec7f 2022-02-08 10:01:48 @localhost
> 
> Fixes: commit 91b228107da3 ("HID: intel-ish-hid: ISH firmware loader
> client driver")
> Signed-off-by: Gwendal Grignou <gwendal@chromium.org>
> Cc: stable@vger.kernel.org
> ---
>  drivers/hid/intel-ish-hid/ishtp-fw-loader.c | 29 +++----------------
> --
>  1 file changed, 3 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/hid/intel-ish-hid/ishtp-fw-loader.c
> b/drivers/hid/intel-ish-hid/ishtp-fw-loader.c
> index e24988586710..16aa030af845 100644
> --- a/drivers/hid/intel-ish-hid/ishtp-fw-loader.c
> +++ b/drivers/hid/intel-ish-hid/ishtp-fw-loader.c
> @@ -661,21 +661,12 @@ static int ish_fw_xfer_direct_dma(struct
> ishtp_cl_data *client_data,
>          */
>         payload_max_size &= ~(L1_CACHE_BYTES - 1);
>  
> -       dma_buf = kmalloc(payload_max_size, GFP_KERNEL | GFP_DMA32);
> +       dma_buf = dma_alloc_coherent(devc, payload_max_size,
> &dma_buf_phy, GFP_KERNEL);
>         if (!dma_buf) {
>                 client_data->flag_retry = true;
>                 return -ENOMEM;
>         }
>  
> -       dma_buf_phy = dma_map_single(devc, dma_buf, payload_max_size,
> -                                    DMA_TO_DEVICE);
> -       if (dma_mapping_error(devc, dma_buf_phy)) {
> -               dev_err(cl_data_to_dev(client_data), "DMA map
> failed\n");
> -               client_data->flag_retry = true;
> -               rv = -ENOMEM;
> -               goto end_err_dma_buf_release;
> -       }
> -
>         ldr_xfer_dma_frag.fragment.hdr.command =
> LOADER_CMD_XFER_FRAGMENT;
>         ldr_xfer_dma_frag.fragment.xfer_mode =
> LOADER_XFER_MODE_DIRECT_DMA;
>         ldr_xfer_dma_frag.ddr_phys_addr = (u64)dma_buf_phy;
> @@ -695,14 +686,7 @@ static int ish_fw_xfer_direct_dma(struct
> ishtp_cl_data *client_data,
>                 ldr_xfer_dma_frag.fragment.size = fragment_size;
>                 memcpy(dma_buf, &fw->data[fragment_offset],
> fragment_size);
>  
> -               dma_sync_single_for_device(devc, dma_buf_phy,
> -                                          payload_max_size,
> -                                          DMA_TO_DEVICE);
> -
Any reason for removal of sync?

Thanks,
Srinivas

> -               /*
> -                * Flush cache here because the
> dma_sync_single_for_device()
> -                * does not do for x86.
> -                */
> +               /* Flush cache to be sure the data is in main memory.
> */
>                 clflush_cache_range(dma_buf, payload_max_size);
>  
>                 dev_dbg(cl_data_to_dev(client_data),
> @@ -725,15 +709,8 @@ static int ish_fw_xfer_direct_dma(struct
> ishtp_cl_data *client_data,
>                 fragment_offset += fragment_size;
>         }
>  
> -       dma_unmap_single(devc, dma_buf_phy, payload_max_size,
> DMA_TO_DEVICE);
> -       kfree(dma_buf);
> -       return 0;
> -
>  end_err_resp_buf_release:
> -       /* Free ISH buffer if not done already, in error case */
> -       dma_unmap_single(devc, dma_buf_phy, payload_max_size,
> DMA_TO_DEVICE);
> -end_err_dma_buf_release:
> -       kfree(dma_buf);
> +       dma_free_coherent(devc, payload_max_size, dma_buf,
> dma_buf_phy);
>         return rv;
>  }
>