Re: [PATCH v3] HID: intel-ish-hid: ISH firmware loader client driver
Hi Nick, Joe
thanks for your comments
Regards
Rushikesh
On Mon, Apr 01, 2019 at 03:17:13PM -0600, Nick Crews wrote:
> I tried to send the last message from my phone, and surprise it wasn't
> formatted correctly, so it may have been marked as spam. repeating
> myself again...
>
> Ah, I guess I was wrong about logging OOM. I hadn’t hear about the
> recommendations against it, but they make sense. Thanks for the
> clarifications!
>
> On Sat, Mar 30, 2019 at 10:27 AM Joe Perches wrote:
> >
> > On Sat, 2019-03-30 at 15:52 +0530, Rushikesh S Kadam wrote:
> > > On Fri, Mar 29, 2019 at 04:30:18PM -0700, Nick Crews wrote:
> > > > On Fri, Mar 29, 2019 at 1:03 PM Rushikesh S Kadam
> > > > wrote:
> > > > > + ldr_xfer_ipc_frag = kzalloc(LOADER_SHIM_IPC_BUF_SIZE,
> > > > > GFP_KERNEL);
> > > > > + if (!ldr_xfer_ipc_frag) {
> > > > Log error here.
> > > The error code is logged in calling function
> > > load_fw_from_host(). Is that good enough?
> > >
> > > I believe the checkpatch script too, would
> > > recommend against adding debug print for ENOMEM
> > > error.
> >
> > The generic kernel allocation functions already do
> > a dump_stack() on OOM conditions when called without
> > __GFP_NOWARN so any additional OOM message isn't
> > particularly useful.
> >
> > > Again, I thought it was against practise to log
> > > "out of memory" debug prints in probe()
> >
> > Or anywhere else given the generic OOM stack dump.
> >
> > > But will add if you tell me this is the right way.
> > >
> > > > > + return -ENOMEM;
> > > > > +
> > > > > + loader_ishtp_cl = ishtp_cl_allocate(cl_device);
> > > > > + if (!loader_ishtp_cl)
> > > >
> > > > log error here
> >
> > The ishtp_cl_allocate function just calls kmalloc then
> > initializes the struct so an additional OOM message
> > isn't useful here either.
> >
> >
--
Re: [PATCH v3] HID: intel-ish-hid: ISH firmware loader client driver
I tried to send the last message from my phone, and surprise it wasn't
formatted correctly, so it may have been marked as spam. repeating
myself again...
Ah, I guess I was wrong about logging OOM. I hadn’t hear about the
recommendations against it, but they make sense. Thanks for the
clarifications!
On Sat, Mar 30, 2019 at 10:27 AM Joe Perches wrote:
>
> On Sat, 2019-03-30 at 15:52 +0530, Rushikesh S Kadam wrote:
> > On Fri, Mar 29, 2019 at 04:30:18PM -0700, Nick Crews wrote:
> > > On Fri, Mar 29, 2019 at 1:03 PM Rushikesh S Kadam
> > > wrote:
> > > > + ldr_xfer_ipc_frag = kzalloc(LOADER_SHIM_IPC_BUF_SIZE,
> > > > GFP_KERNEL);
> > > > + if (!ldr_xfer_ipc_frag) {
> > > Log error here.
> > The error code is logged in calling function
> > load_fw_from_host(). Is that good enough?
> >
> > I believe the checkpatch script too, would
> > recommend against adding debug print for ENOMEM
> > error.
>
> The generic kernel allocation functions already do
> a dump_stack() on OOM conditions when called without
> __GFP_NOWARN so any additional OOM message isn't
> particularly useful.
>
> > Again, I thought it was against practise to log
> > "out of memory" debug prints in probe()
>
> Or anywhere else given the generic OOM stack dump.
>
> > But will add if you tell me this is the right way.
> >
> > > > + return -ENOMEM;
> > > > +
> > > > + loader_ishtp_cl = ishtp_cl_allocate(cl_device);
> > > > + if (!loader_ishtp_cl)
> > >
> > > log error here
>
> The ishtp_cl_allocate function just calls kmalloc then
> initializes the struct so an additional OOM message
> isn't useful here either.
>
>
Re: [PATCH v3] HID: intel-ish-hid: ISH firmware loader client driver
On Sat, 2019-03-30 at 15:52 +0530, Rushikesh S Kadam wrote:
> On Fri, Mar 29, 2019 at 04:30:18PM -0700, Nick Crews wrote:
> > On Fri, Mar 29, 2019 at 1:03 PM Rushikesh S Kadam
> > wrote:
> > > + ldr_xfer_ipc_frag = kzalloc(LOADER_SHIM_IPC_BUF_SIZE, GFP_KERNEL);
> > > + if (!ldr_xfer_ipc_frag) {
> > Log error here.
> The error code is logged in calling function
> load_fw_from_host(). Is that good enough?
>
> I believe the checkpatch script too, would
> recommend against adding debug print for ENOMEM
> error.
The generic kernel allocation functions already do
a dump_stack() on OOM conditions when called without
__GFP_NOWARN so any additional OOM message isn't
particularly useful.
> Again, I thought it was against practise to log
> "out of memory" debug prints in probe()
Or anywhere else given the generic OOM stack dump.
> But will add if you tell me this is the right way.
>
> > > + return -ENOMEM;
> > > +
> > > + loader_ishtp_cl = ishtp_cl_allocate(cl_device);
> > > + if (!loader_ishtp_cl)
> >
> > log error here
The ishtp_cl_allocate function just calls kmalloc then
initializes the struct so an additional OOM message
isn't useful here either.
Re: [PATCH v3] HID: intel-ish-hid: ISH firmware loader client driver
Hi Nick
I've few comments below about your suggestions,
On Fri, Mar 29, 2019 at 04:30:18PM -0700, Nick Crews wrote:
> On Fri, Mar 29, 2019 at 1:03 PM Rushikesh S Kadam
> wrote:
> >
> > +/**
> > + * ish_fw_xfer_ishtp() Loads ISH firmware using ishtp interface
> > + * @client_data: Client data instance
> > + * @fw:Pointer to firmware data struct in host
> > memory
> > + *
> > + * This function uses ISH-TP to transfer ISH firmware from host to
> > + * ISH SRAM. Lower layers may use IPC or DMA depending on firmware
> > + * support.
> > + *
> > + * Return: 0 for success, negative error code for failure.
> > + */
> > +static int ish_fw_xfer_ishtp(struct ishtp_cl_data *client_data,
> > +const struct firmware *fw)
> > +{
> > + int rv;
> > + u32 fragment_offset, fragment_size, payload_max_size;
> > + struct loader_xfer_ipc_fragment *ldr_xfer_ipc_frag;
> > + struct loader_msg_hdr ldr_xfer_ipc_ack;
> > +
> > + payload_max_size =
> > + LOADER_SHIM_IPC_BUF_SIZE - IPC_FRAGMENT_DATA_PREAMBLE;
> > +
> > + ldr_xfer_ipc_frag = kzalloc(LOADER_SHIM_IPC_BUF_SIZE, GFP_KERNEL);
> > + if (!ldr_xfer_ipc_frag) {
>
> Log error here.
>
The error code is logged in calling function
load_fw_from_host(). Is that good enough?
I believe the checkpatch script too, would
recommend against adding debug print for ENOMEM
error.
> > + /*
> > +* payload_max_size should be set to minimum of
> > +* (1) Size of firmware to be loaded,
> > +* (2) Max DMA buffer size supported by Shim firmware,
> > +* (3) DMA buffer size limit set by boot_param dma_buf_size_limit.
> > +*/
> > + payload_max_size = min3(fw->size,
> > + (size_t)shim_fw_buf_size,
> > + (size_t)dma_buf_size_limit);
> > +
> > + /*
> > +* Buffer size should be multiple of cacheline size
> > +* if it's not, select the previous cacheline boundary.
> > +*/
> > + payload_max_size &= ~(L1_CACHE_BYTES - 1);
> > +
> > + dma_buf = kmalloc(payload_max_size, GFP_KERNEL | GFP_DMA32);
> > + if (!dma_buf) {
>
> Log error here.
Same comment as above.
> > +static int load_fw_from_host(struct ishtp_cl_data *client_data)
> > +{
> > + int rv;
> > + u32 xfer_mode;
> > + char *filename;
> > + const struct firmware *fw;
> > + struct shim_fw_info fw_info;
> > + struct ishtp_cl *loader_ishtp_cl = client_data->loader_ishtp_cl;
> > +
> > + client_data->flag_retry = false;
> > +
> > + filename = kzalloc(FILENAME_SIZE, GFP_KERNEL);
> > + if (!filename) {
> > + client_data->flag_retry = true;
> > + rv = -ENOMEM;
>
> log error here
>
We log the error code below.
> > +/**
> > + * loader_ishtp_cl_probe() - ISH-TP client driver probe
> > + * @cl_device: ISH-TP client device instance
> > + *
> > + * This function gets called on device create on ISH-TP bus
> > + *
> > + * Return: 0 for success, negative error code for failure
> > + */
> > +static int loader_ishtp_cl_probe(struct ishtp_cl_device *cl_device)
> > +{
> > + struct ishtp_cl *loader_ishtp_cl;
> > + struct ishtp_cl_data *client_data;
> > + int rv;
> > +
> > + client_data = devm_kzalloc(ishtp_device(cl_device),
> > + sizeof(*client_data),
> > + GFP_KERNEL);
> > + if (!client_data)
>
> log error here
Again, I thought it was against practise to log
"out of memory" debug prints in probe()
But will add if you tell me this is the right way.
>
> > + return -ENOMEM;
> > +
> > + loader_ishtp_cl = ishtp_cl_allocate(cl_device);
> > + if (!loader_ishtp_cl)
>
> log error here
Same comment.
Thanks
Rushikesh
>
> > + return -ENOMEM;
> > +
> > + ishtp_set_drvdata(cl_device, loader_ishtp_cl);
> > + ishtp_set_client_data(loader_ishtp_cl, client_data);
> > + client_data->loader_ishtp_cl = loader_ishtp_cl;
> > + client_data->cl_device = cl_device;
> > +
> > + init_waitqueue_head(_data->response.wait_queue);
> > +
> > + INIT_WORK(_data->work_ishtp_reset,
> > + reset_handler);
> > + INIT_WORK(_data->work_fw_load,
> > + load_fw_from_host_handler);
> > +
> > + rv = loader_init(loader_ishtp_cl, 0);
> > + if (rv < 0) {
> > + ishtp_cl_free(loader_ishtp_cl);
> > + return rv;
> > + }
> > + ishtp_get_device(cl_device);
> > +
> > + client_data->retry_count = 0;
> > +
> > + /* ISH firmware loading from host */
> > + schedule_work(_data->work_fw_load);
> > +
> > + return 0;
> > +}
> > +
> > +/**
> > + * loader_ishtp_cl_remove() - ISH-TP client driver remove
> > + * @cl_device: ISH-TP client device instance
> > + *
> > + *
Re: [PATCH v3] HID: intel-ish-hid: ISH firmware loader client driver
On Fri, Mar 29, 2019 at 1:03 PM Rushikesh S Kadam wrote: > > This driver adds support for loading Intel Integrated > Sensor Hub (ISH) firmware from host file system to ISH > SRAM and start execution. > > At power-on, the ISH subsystem shall boot to an interim > Shim loader-firmware, which shall expose an ISHTP loader > device. > > The driver implements an ISHTP client that communicates > with the Shim ISHTP loader device over the intel-ish-hid > stack, to download the main ISH firmware. > > Signed-off-by: Rushikesh S Kadam > --- > The patches are baselined to hid git tree, branch for-5.2/ish > https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/log/?h=for-5.2/ish > > v3 > - Moved a couple of sanity checks from loader_cl_send() to >process_recv(). > - Several minor changes to address review comments. > > v2 > - Change loader_cl_send() so that the calling function >shall allocate and pass the buffer to be used for >receiving firwmare response data. Corresponding changes >in calling function and process_recv(). > - Introduced struct response_info to encapsulate and pass >data between from the process_recv() callback to >calling function loader_cl_send(). > - Keep count of host firmware load retries, and fail after >3 unsuccessful attempts. > - Dropped report_bad_packets() function previously used for >keeping count of bad packets. > - Inlined loader_ish_hw_reset()'s functionality > > v1 > - Initial version. > > drivers/hid/Makefile|1 + > drivers/hid/intel-ish-hid/Kconfig | 15 + > drivers/hid/intel-ish-hid/Makefile |3 + > drivers/hid/intel-ish-hid/ishtp-fw-loader.c | 1088 > +++ > 4 files changed, 1107 insertions(+) > create mode 100644 drivers/hid/intel-ish-hid/ishtp-fw-loader.c > > diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile > index 170163b..d8d393e 100644 > --- a/drivers/hid/Makefile > +++ b/drivers/hid/Makefile > @@ -134,3 +134,4 @@ obj-$(CONFIG_USB_KBD) += usbhid/ > obj-$(CONFIG_I2C_HID) += i2c-hid/ > > obj-$(CONFIG_INTEL_ISH_HID)+= intel-ish-hid/ > +obj-$(INTEL_ISH_FIRMWARE_DOWNLOADER) += intel-ish-hid/ > diff --git a/drivers/hid/intel-ish-hid/Kconfig > b/drivers/hid/intel-ish-hid/Kconfig > index 519e4c8..786adbc 100644 > --- a/drivers/hid/intel-ish-hid/Kconfig > +++ b/drivers/hid/intel-ish-hid/Kconfig > @@ -14,4 +14,19 @@ config INTEL_ISH_HID > Broxton and Kaby Lake. > > Say Y here if you want to support Intel ISH. If unsure, say N. > + > +config INTEL_ISH_FIRMWARE_DOWNLOADER > + tristate "Host Firmware Load feature for Intel ISH" > + depends on INTEL_ISH_HID > + depends on X86 > + help > + The Integrated Sensor Hub (ISH) enables the kernel to offload > + sensor polling and algorithm processing to a dedicated low power > + processor in the chipset. > + > + The Host Firmware Load feature adds support to load the ISH > + firmware from host file system at boot. > + > + Say M here if you want to support Host Firmware Loading feature > + for Intel ISH. If unsure, say N. > endmenu > diff --git a/drivers/hid/intel-ish-hid/Makefile > b/drivers/hid/intel-ish-hid/Makefile > index 825b70a..2de97e4 100644 > --- a/drivers/hid/intel-ish-hid/Makefile > +++ b/drivers/hid/intel-ish-hid/Makefile > @@ -20,4 +20,7 @@ obj-$(CONFIG_INTEL_ISH_HID) += intel-ishtp-hid.o > intel-ishtp-hid-objs := ishtp-hid.o > intel-ishtp-hid-objs += ishtp-hid-client.o > > +obj-$(CONFIG_INTEL_ISH_FIRMWARE_DOWNLOADER) += intel-ishtp-loader.o > +intel-ishtp-loader-objs += ishtp-fw-loader.o > + > ccflags-y += -Idrivers/hid/intel-ish-hid/ishtp > diff --git a/drivers/hid/intel-ish-hid/ishtp-fw-loader.c > b/drivers/hid/intel-ish-hid/ishtp-fw-loader.c > new file mode 100644 > index 000..656dec0 > --- /dev/null > +++ b/drivers/hid/intel-ish-hid/ishtp-fw-loader.c > @@ -0,0 +1,1088 @@ > +// SPDX-License-Identifier: GPL-2.0 > +/* > + * ISH-TP client driver for ISH firmware loading > + * > + * Copyright (c) 2019, Intel Corporation. > + */ > + > +#include > +#include > +#include > +#include > +#include > +#include > + > +/* Number of times we attempt to load the firmware before giving up */ > +#define MAX_LOAD_ATTEMPTS 3 > + > +/* ISH TX/RX ring buffer pool size */ > +#define LOADER_CL_RX_RING_SIZE 1 > +#define LOADER_CL_TX_RING_SIZE 1 > + > +/* > + * ISH Shim firmware loader reserves 4 Kb buffer in SRAM. The buffer is > + * used to temporarily hold the data transferred from host to Shim > + * firmware loader. Reason for the odd size of 3968 bytes? Each IPC > + * transfer is 128 bytes (= 4 bytes header + 124 bytes payload). So the > + * 4 Kb buffer can hold maximum of 32 IPC transfers, which means we can > + * have a max payload of 3968 bytes (= 32 x 124 payload). > + */ > +#define LOADER_SHIM_IPC_BUF_SIZE
[PATCH v3] HID: intel-ish-hid: ISH firmware loader client driver
This driver adds support for loading Intel Integrated Sensor Hub (ISH) firmware from host file system to ISH SRAM and start execution. At power-on, the ISH subsystem shall boot to an interim Shim loader-firmware, which shall expose an ISHTP loader device. The driver implements an ISHTP client that communicates with the Shim ISHTP loader device over the intel-ish-hid stack, to download the main ISH firmware. Signed-off-by: Rushikesh S Kadam --- The patches are baselined to hid git tree, branch for-5.2/ish https://git.kernel.org/pub/scm/linux/kernel/git/hid/hid.git/log/?h=for-5.2/ish v3 - Moved a couple of sanity checks from loader_cl_send() to process_recv(). - Several minor changes to address review comments. v2 - Change loader_cl_send() so that the calling function shall allocate and pass the buffer to be used for receiving firwmare response data. Corresponding changes in calling function and process_recv(). - Introduced struct response_info to encapsulate and pass data between from the process_recv() callback to calling function loader_cl_send(). - Keep count of host firmware load retries, and fail after 3 unsuccessful attempts. - Dropped report_bad_packets() function previously used for keeping count of bad packets. - Inlined loader_ish_hw_reset()'s functionality v1 - Initial version. drivers/hid/Makefile|1 + drivers/hid/intel-ish-hid/Kconfig | 15 + drivers/hid/intel-ish-hid/Makefile |3 + drivers/hid/intel-ish-hid/ishtp-fw-loader.c | 1088 +++ 4 files changed, 1107 insertions(+) create mode 100644 drivers/hid/intel-ish-hid/ishtp-fw-loader.c diff --git a/drivers/hid/Makefile b/drivers/hid/Makefile index 170163b..d8d393e 100644 --- a/drivers/hid/Makefile +++ b/drivers/hid/Makefile @@ -134,3 +134,4 @@ obj-$(CONFIG_USB_KBD) += usbhid/ obj-$(CONFIG_I2C_HID) += i2c-hid/ obj-$(CONFIG_INTEL_ISH_HID)+= intel-ish-hid/ +obj-$(INTEL_ISH_FIRMWARE_DOWNLOADER) += intel-ish-hid/ diff --git a/drivers/hid/intel-ish-hid/Kconfig b/drivers/hid/intel-ish-hid/Kconfig index 519e4c8..786adbc 100644 --- a/drivers/hid/intel-ish-hid/Kconfig +++ b/drivers/hid/intel-ish-hid/Kconfig @@ -14,4 +14,19 @@ config INTEL_ISH_HID Broxton and Kaby Lake. Say Y here if you want to support Intel ISH. If unsure, say N. + +config INTEL_ISH_FIRMWARE_DOWNLOADER + tristate "Host Firmware Load feature for Intel ISH" + depends on INTEL_ISH_HID + depends on X86 + help + The Integrated Sensor Hub (ISH) enables the kernel to offload + sensor polling and algorithm processing to a dedicated low power + processor in the chipset. + + The Host Firmware Load feature adds support to load the ISH + firmware from host file system at boot. + + Say M here if you want to support Host Firmware Loading feature + for Intel ISH. If unsure, say N. endmenu diff --git a/drivers/hid/intel-ish-hid/Makefile b/drivers/hid/intel-ish-hid/Makefile index 825b70a..2de97e4 100644 --- a/drivers/hid/intel-ish-hid/Makefile +++ b/drivers/hid/intel-ish-hid/Makefile @@ -20,4 +20,7 @@ obj-$(CONFIG_INTEL_ISH_HID) += intel-ishtp-hid.o intel-ishtp-hid-objs := ishtp-hid.o intel-ishtp-hid-objs += ishtp-hid-client.o +obj-$(CONFIG_INTEL_ISH_FIRMWARE_DOWNLOADER) += intel-ishtp-loader.o +intel-ishtp-loader-objs += ishtp-fw-loader.o + ccflags-y += -Idrivers/hid/intel-ish-hid/ishtp diff --git a/drivers/hid/intel-ish-hid/ishtp-fw-loader.c b/drivers/hid/intel-ish-hid/ishtp-fw-loader.c new file mode 100644 index 000..656dec0 --- /dev/null +++ b/drivers/hid/intel-ish-hid/ishtp-fw-loader.c @@ -0,0 +1,1088 @@ +// SPDX-License-Identifier: GPL-2.0 +/* + * ISH-TP client driver for ISH firmware loading + * + * Copyright (c) 2019, Intel Corporation. + */ + +#include +#include +#include +#include +#include +#include + +/* Number of times we attempt to load the firmware before giving up */ +#define MAX_LOAD_ATTEMPTS 3 + +/* ISH TX/RX ring buffer pool size */ +#define LOADER_CL_RX_RING_SIZE 1 +#define LOADER_CL_TX_RING_SIZE 1 + +/* + * ISH Shim firmware loader reserves 4 Kb buffer in SRAM. The buffer is + * used to temporarily hold the data transferred from host to Shim + * firmware loader. Reason for the odd size of 3968 bytes? Each IPC + * transfer is 128 bytes (= 4 bytes header + 124 bytes payload). So the + * 4 Kb buffer can hold maximum of 32 IPC transfers, which means we can + * have a max payload of 3968 bytes (= 32 x 124 payload). + */ +#define LOADER_SHIM_IPC_BUF_SIZE 3968 + +/** + * enum ish_loader_commands - ISH loader host commands. + * LOADER_CMD_XFER_QUERY Query the Shim firmware loader for + * capabilities + * LOADER_CMD_XFER_FRAGMENTTransfer one firmware image fragment at a + * time. The command
