* RE: [PATCH 0/6] mfd: da9063: remove platform_data
From: Steve Twiss @ 2019-03-29 11:28 UTC (permalink / raw)
To: 'Wolfram Sang', linux-kernel@vger.kernel.org
Cc: linux-renesas-soc@vger.kernel.org, Lee Jones, Mark Brown,
Support Opensource, linux-input@vger.kernel.org
In-Reply-To: <VI1PR10MB235256E1840C1E594ECDC0A5FE5A0@VI1PR10MB2352.EURPRD10.PROD.OUTLOOK.COM>
Caveat.
On 29 March 2019 11:02 Steve Twiss, wrote:
> Subject: RE: [PATCH 0/6] mfd: da9063: remove platform_data
>
[...]
>
> I've acked-by and tested-by for all the patches now. Apologies because it took
> such a long time to get around to looking at this.
>
> (For my benefit) I regression tested against v5.0 and v5.1-rc1, ok.
These were tested with two extra patches that are not in vanilla v5.0 or v5.1.-rc1.
One is in discussion and one is applied to linux-next,
https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next-history.git/commit/?id=70b464918e5331e488058870fcc6821d54c4e541
E-mail threads are here:
https://lore.kernel.org/lkml/20190318163132.1BB633FBA2@swsrvapps-01.diasemi.com/
https://lore.kernel.org/lkml/20190320120604.7D99B3FBE9@swsrvapps-01.diasemi.com/
Regards,
Steve
^ permalink raw reply
* Re: [PATCH 0/6] mfd: da9063: remove platform_data
From: Wolfram Sang @ 2019-03-29 11:27 UTC (permalink / raw)
To: Steve Twiss
Cc: Wolfram Sang, linux-kernel@vger.kernel.org,
linux-renesas-soc@vger.kernel.org, Lee Jones, Mark Brown,
Support Opensource, linux-input@vger.kernel.org
In-Reply-To: <VI1PR10MB235256E1840C1E594ECDC0A5FE5A0@VI1PR10MB2352.EURPRD10.PROD.OUTLOOK.COM>
[-- Attachment #1: Type: text/plain, Size: 203 bytes --]
> I've acked-by and tested-by for all the patches now. Apologies because it took
> such a long time to get around to looking at this.
Thanks for the testing! I think 10 days is actually quite fast :)
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* RE: [PATCH 0/6] mfd: da9063: remove platform_data
From: Steve Twiss @ 2019-03-29 11:02 UTC (permalink / raw)
To: Wolfram Sang, linux-kernel@vger.kernel.org
Cc: linux-renesas-soc@vger.kernel.org, Lee Jones, Mark Brown,
Support Opensource, linux-input@vger.kernel.org
In-Reply-To: <20190318154759.21978-1-wsa+renesas@sang-engineering.com>
Hi,
On 18 March 2019 15:48 Wolfram Sang wrote:
> Subject: [PATCH 0/6] mfd: da9063: remove platform_data
>
> When working with this driver while debugging something else, I noticed that
> there are no in-kernel users of platform_data anymore. Removing the support
> simplifies the code and gets rid of quite some lines. The biggest refactoring
> went to the regulator driver which could maybe benefit from more
> refactorization. However, there is no in-kernel user of that driver, so no
> testing. I left it at this stage. I think the changes are still at a level
> where review and build-testing will give enough confidence.
>
> Besides the regulator thing, it was tested on a Renesas Lager board (R-Car H2).
> No regressions encountered. buildbot was happy, too.
>
> The patches are based on v5.1-rc1. I'd vote for all of them going through the
> MFD tree. Looking forward to comments!
>
> Wolfram
I've acked-by and tested-by for all the patches now. Apologies because it took
such a long time to get around to looking at this.
(For my benefit) I regression tested against v5.0 and v5.1-rc1, ok.
Thanks,
Steve
^ permalink raw reply
* RE: [PATCH 2/6] input: da9063_onkey: remove platform_data support
From: Steve Twiss @ 2019-03-29 10:58 UTC (permalink / raw)
To: Wolfram Sang, Dmitry Torokhov
Cc: LKML, linux-renesas-soc@vger.kernel.org, Lee Jones, Mark Brown,
Support Opensource, linux-input@vger.kernel.org
In-Reply-To: <20190318154759.21978-3-wsa+renesas@sang-engineering.com>
Hi,
On 18 March 2019 15:48, Wolfram Sang wrote:
> Subject: [PATCH 2/6] input: da9063_onkey: remove platform_data support
>
> There are no in-kernel users anymore, so remove this outdated interface.
>
> Signed-off-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> ---
> drivers/input/misc/da9063_onkey.c | 11 ++---------
> 1 file changed, 2 insertions(+), 9 deletions(-)
Thanks!
Acked-by: Steve Twiss <stwiss.opensource@diasemi.com>
Tested-by: Steve Twiss <stwiss.opensource@diasemi.com>
Regards,
Steve
^ permalink raw reply
* Re: Re: [PATCH v3 1/4] drm/bridge: sil_sii8620: depend on INPUT instead of selecting it.
From: Life is hard, and then you die @ 2019-03-29 9:22 UTC (permalink / raw)
To: Andrzej Hajda
Cc: Dmitry Torokhov, Henrik Rydberg, Andy Shevchenko,
Sergey Senozhatsky, Steven Rostedt, Greg Kroah-Hartman,
Rafael J. Wysocki, Lukas Wunner, Federico Lorenzi, linux-input,
linux-kernel, Inki Dae, dri-devel@lists.freedesktop.org
In-Reply-To: <35bf8a61-b84c-3aae-00de-14637d37547b@samsung.com>
On Thu, Mar 28, 2019 at 12:48:39PM +0100, Andrzej Hajda wrote:
> On 28.03.2019 01:07, Life is hard, and then you die wrote:
> >
> > On Wed, Mar 27, 2019 at 03:13:37PM +0100, Andrzej Hajda wrote:
> >> +cc: dri-devel
[snip]
> > It seems your mail client doesn't like me :-) I got neither of your
> > responses. Sorry, I should've checked the archives when I didn't hear
> > anything. In any case thank you for your review, and I will update
> > that patch and send out a new version shortly.
>
> I see where is the problem: Your mail client (mutt I suppose) sets
> Mail-Followup-To header to all recipients (To and Cc) without the
> sender. And my client (thunderbird) after pressing "Reply-All" checks
> for Mail-Followup-To field, if present it uses only it to set
> recipients, so in your case it does not response to the original author.
>
> I do not know which mail client works incorrectly in this case, but for
> sure it is not what we want :)
>
> I do not know how to solve the issue in thunderbird, maybe mutt is more
> configurable?
Thanks for debugging this! I'm sorry, but it looks like this was an
error on my part. So for anybody else using mutt: by default, mutt
will add a Mail-Followup-To header without your own address when
sending mail to any lists you are subscribed to, as defined by the
'subscribe' command (to avoid receiving replies in duplicate, i.e.
once via to/cc and once via the list). Unfortunately I was using a
regex there that included all freedesktop.org lists, not just ones I'm
actually subscribed to, and hence it thought I was subscribed
dri-devel too, leading to the this screwup.
Apologies for this - I guess even after more than 2 decades of mutt
and mailing lists I still manage to not know all its ins and outs.
But it should be fixed now.
Cheers,
Ronald
^ permalink raw reply
* Re: [PATCH v4] HID: core: move Usage Page concatenation to Main item
From: Benjamin Tissoires @ 2019-03-29 7:51 UTC (permalink / raw)
To: Junge, Terry
Cc: Nicolas Saenz Julienne, Jiri Kosina, oneukum@suse.de,
linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <BYAPR02MB560797BC61D1AE47761139B48A5A0@BYAPR02MB5607.namprd02.prod.outlook.com>
On Fri, Mar 29, 2019 at 1:48 AM Junge, Terry <terry.junge@poly.com> wrote:
>
> On Thursday, March 28, 2019 6:49 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
> >>
> >> Hi Nicolas,
> >>
> >> V4 looks good to me.
> >
> >Looks good to me too.
> >
> >Terry, can I consider this as a formal Rev-by you?
>
> Benjamin,
>
> Yes, thank you. What is the normal way of formally indicating Rev-by?
>
Thanks.
So giving a rev-by is described in
https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/tree/Documentation/process/submitting-patches.rst
section 13.
Basically, you just answer a patch with: "Reviewed-by: Your Name
<your@address.sth>" (without quotes).
This has 2 benefits: it's formal, and everybody understands it, and
patchwork (at https://patchwork.kernel.org/patch/10873179/ for this
patch) will catch it. Then with the patchwork client, I can apply the
patch without having to manually add your tag :) (no need to resend
one here, unless you want to give a try, I'll apply it manually).
Cheers,
Benjamin
^ permalink raw reply
* [PATCH 2/2] input: keyboard: imx: make sure keyboard can always wake up system
From: Anson Huang @ 2019-03-29 7:00 UTC (permalink / raw)
To: dmitry.torokhov@gmail.com, shawnguo@kernel.org,
s.hauer@pengutronix.de, kernel@pengutronix.de, festevam@gmail.com,
linux-input@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Cc: dl-linux-imx
In-Reply-To: <1553842562-8481-1-git-send-email-Anson.Huang@nxp.com>
There are several scenarios that keyboard can NOT wake up system
from suspend, e.g., if a keyboard is depressed between system
device suspend phase and device noirq suspend phase, the keyboard
ISR will be called and both keyboard depress and release interrupts
will be disabled, then keyboard will no longer be able to wake up
system. Another scenario would be, if a keyboard is kept depressed,
and then system goes into suspend, the expected behavior would be
when keyboard is released, system will be waked up, but current
implementation can NOT achieve that, because both depress and release
interrupts are disabled in ISR, and the event check is still in
progress.
To fix these issues, need to make sure keyboard's depress or release
interrupt is enabled after noirq device suspend phase, this patch
moves the suspend/resume callback to noirq suspend/resume phase, and
enable the corresponding interrupt according to current keyboard status.
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
drivers/input/keyboard/imx_keypad.c | 18 ++++++++++++++----
1 file changed, 14 insertions(+), 4 deletions(-)
diff --git a/drivers/input/keyboard/imx_keypad.c b/drivers/input/keyboard/imx_keypad.c
index 7e32c36..9ae2090 100644
--- a/drivers/input/keyboard/imx_keypad.c
+++ b/drivers/input/keyboard/imx_keypad.c
@@ -521,11 +521,12 @@ static int imx_keypad_probe(struct platform_device *pdev)
return 0;
}
-static int __maybe_unused imx_kbd_suspend(struct device *dev)
+static int __maybe_unused imx_kbd_noirq_suspend(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
struct imx_keypad *kbd = platform_get_drvdata(pdev);
struct input_dev *input_dev = kbd->input_dev;
+ unsigned short reg_val = readw(kbd->mmio_base + KPSR);
/* imx kbd can wake up system even clock is disabled */
mutex_lock(&input_dev->mutex);
@@ -535,13 +536,20 @@ static int __maybe_unused imx_kbd_suspend(struct device *dev)
mutex_unlock(&input_dev->mutex);
- if (device_may_wakeup(&pdev->dev))
+ if (device_may_wakeup(&pdev->dev)) {
+ if (reg_val & KBD_STAT_KPKD)
+ reg_val |= KBD_STAT_KRIE;
+ if (reg_val & KBD_STAT_KPKR)
+ reg_val |= KBD_STAT_KDIE;
+ writew(reg_val, kbd->mmio_base + KPSR);
+
enable_irq_wake(kbd->irq);
+ }
return 0;
}
-static int __maybe_unused imx_kbd_resume(struct device *dev)
+static int __maybe_unused imx_kbd_noirq_resume(struct device *dev)
{
struct platform_device *pdev = to_platform_device(dev);
struct imx_keypad *kbd = platform_get_drvdata(pdev);
@@ -565,7 +573,9 @@ static int __maybe_unused imx_kbd_resume(struct device *dev)
return ret;
}
-static SIMPLE_DEV_PM_OPS(imx_kbd_pm_ops, imx_kbd_suspend, imx_kbd_resume);
+static const struct dev_pm_ops imx_kbd_pm_ops = {
+ SET_NOIRQ_SYSTEM_SLEEP_PM_OPS(imx_kbd_noirq_suspend, imx_kbd_noirq_resume)
+};
static struct platform_driver imx_keypad_driver = {
.driver = {
--
2.7.4
^ permalink raw reply related
* [PATCH 1/2] input: keyboard: imx: no need to control interrupt status in event check
From: Anson Huang @ 2019-03-29 7:00 UTC (permalink / raw)
To: dmitry.torokhov@gmail.com, shawnguo@kernel.org,
s.hauer@pengutronix.de, kernel@pengutronix.de, festevam@gmail.com,
linux-input@vger.kernel.org, linux-arm-kernel@lists.infradead.org,
linux-kernel@vger.kernel.org
Cc: dl-linux-imx
There is no need to enable release interrupt and disable depress
interrupt in event check, as a timer is setup for checking these
events rather than interrupts.
Signed-off-by: Anson Huang <Anson.Huang@nxp.com>
---
drivers/input/keyboard/imx_keypad.c | 5 -----
1 file changed, 5 deletions(-)
diff --git a/drivers/input/keyboard/imx_keypad.c b/drivers/input/keyboard/imx_keypad.c
index 539cb67..7e32c36 100644
--- a/drivers/input/keyboard/imx_keypad.c
+++ b/drivers/input/keyboard/imx_keypad.c
@@ -276,11 +276,6 @@ static void imx_keypad_check_for_events(struct timer_list *t)
reg_val = readw(keypad->mmio_base + KPSR);
reg_val |= KBD_STAT_KPKR | KBD_STAT_KRSS;
writew(reg_val, keypad->mmio_base + KPSR);
-
- reg_val = readw(keypad->mmio_base + KPSR);
- reg_val |= KBD_STAT_KRIE;
- reg_val &= ~KBD_STAT_KDIE;
- writew(reg_val, keypad->mmio_base + KPSR);
}
}
--
2.7.4
^ permalink raw reply related
* Re: [PATCH v2] HID: intel-ish-hid: ISH firmware loader client driver
From: Nick Crews @ 2019-03-29 6:02 UTC (permalink / raw)
To: Rushikesh S Kadam
Cc: Srinivas Pandruvada, benjamin.tissoires, jikos, jettrink,
Gwendal Grignou, linux-kernel, linux-input
In-Reply-To: <1553804456-5329-1-git-send-email-rushikesh.s.kadam@intel.com>
This is so close! There are just one or two tiny things.
On Thu, Mar 28, 2019 at 1:20 PM Rushikesh S Kadam
<rushikesh.s.kadam@intel.com> 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 <rushikesh.s.kadam@intel.com>
> ---
> 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
>
> The v2 revision primarily address review comments received on
> the v1 patch.
>
> 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 | 1084 +++++++++++++++++++++++++++
> 4 files changed, 1103 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 0000000..8685fa6
> --- /dev/null
> +++ b/drivers/hid/intel-ish-hid/ishtp-fw-loader.c
> @@ -0,0 +1,1084 @@
> +// SPDX-License-Identifier: GPL-2.0
> +/*
> + * ISH-TP client driver for ISH firmware loading
> + *
> + * Copyright (c) 2019, Intel Corporation.
> + */
> +
> +#include <linux/firmware.h>
> +#include <linux/module.h>
> +#include <linux/pci.h>
> +#include <linux/intel-ish-client-if.h>
> +#include <linux/property.h>
> +#include <asm/cacheflush.h>
> +
> +/* 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_FRAGMENT Transfer one firmware image fragment at a
> + * time. The command may be executed
> + * multiple times until the entire firmware
> + * image is downloaded to SRAM.
> + * LOADER_CMD_START Start executing the main firmware.
> + */
> +enum ish_loader_commands {
> + LOADER_CMD_XFER_QUERY = 0,
> + LOADER_CMD_XFER_FRAGMENT,
> + LOADER_CMD_START,
> +};
> +
> +/* Command bit mask */
> +#define CMD_MASK GENMASK(6, 0)
> +#define IS_RESPONSE BIT(7)
> +
> +/*
> + * ISH firmware max delay for one transmit failure is 1 Hz,
> + * and firmware will retry 2 times, so 3 Hz is used for timeout.
> + */
> +#define ISHTP_SEND_TIMEOUT (3 * HZ)
> +
> +/*
> + * Loader transfer modes:
> + *
> + * LOADER_XFER_MODE_ISHTP mode uses the existing ISH-TP mechanism to
> + * transfer data. This may use IPC or DMA if supported in firmware.
> + * The buffer size is limited to 4 Kb by the IPC/ISH-TP protocol for
> + * both IPC & DMA (legacy).
> + *
> + * LOADER_XFER_MODE_DIRECT_DMA - firmware loading is a bit different
> + * from the sensor data streaming. Here we download a large (300+ Kb)
> + * image directly to ISH SRAM memory. There is limited benefit of
> + * DMA'ing 300 Kb image in 4 Kb chucks limit. Hence, we introduce
> + * this "direct dma" mode, where we do not use ISH-TP for DMA, but
> + * instead manage the DMA directly in kernel driver and Shim firmware
> + * loader (allocate buffer, break in chucks and transfer). This allows
> + * to overcome 4 Kb limit, and optimize the data flow path in firmware.
> + */
> +#define LOADER_XFER_MODE_DIRECT_DMA BIT(0)
> +#define LOADER_XFER_MODE_ISHTP BIT(1)
> +
> +/* ISH Transport Loader client unique GUID */
> +static const guid_t loader_ishtp_guid =
> + GUID_INIT(0xc804d06a, 0x55bd, 0x4ea7,
> + 0xad, 0xed, 0x1e, 0x31, 0x22, 0x8c, 0x76, 0xdc);
> +
> +#define FILENAME_SIZE 256
> +
> +/*
> + * The firmware loading latency will be minimum if we can DMA the
> + * entire ISH firmware image in one go. This requires that we allocate
> + * a large DMA buffer in kernel, which could be problematic on some
> + * platforms. So here we limit the DMA buffer size via a module_param.
> + * We default to 4 pages, but a customer can set it to higher limit if
> + * deemed appropriate for his platform.
> + */
> +static int dma_buf_size_limit = 4 * PAGE_SIZE;
> +
> +/**
> + * struct loader_msg_hdr - Header for ISH Loader commands.
> + * @command: LOADER_CMD* commands. Bit 7 is the response.
> + * @status: Command response status. Non 0, is error
> + * condition.
> + *
> + * This structure is used as header for every command/data sent/received
> + * between Host driver and ISH Shim firmware loader.
> + */
> +struct loader_msg_hdr {
> + u8 command;
> + u8 reserved[2];
> + u8 status;
> +} __packed;
> +
> +struct loader_xfer_query {
> + struct loader_msg_hdr hdr;
> + u32 image_size;
> +} __packed;
> +
> +struct ish_fw_version {
> + u16 major;
> + u16 minor;
> + u16 hotfix;
> + u16 build;
> +} __packed;
> +
> +union loader_version {
> + u32 value;
> + struct {
> + u8 major;
> + u8 minor;
> + u8 hotfix;
> + u8 build;
> + };
> +} __packed;
> +
> +struct loader_capability {
> + u32 max_fw_image_size;
> + u32 xfer_mode;
> + u32 max_dma_buf_size; /* only for dma mode, multiples of cacheline */
> +} __packed;
> +
> +struct shim_fw_info {
> + struct ish_fw_version ish_fw_version;
> + u32 protocol_version;
> + union loader_version ldr_version;
> + struct loader_capability ldr_capability;
> +} __packed;
> +
> +struct loader_xfer_query_response {
> + struct loader_msg_hdr hdr;
> + struct shim_fw_info fw_info;
> +} __packed;
> +
> +struct loader_xfer_fragment {
> + struct loader_msg_hdr hdr;
> + u32 xfer_mode;
> + u32 offset;
> + u32 size;
> + u32 is_last;
> +} __packed;
> +
> +struct loader_xfer_ipc_fragment {
> + struct loader_xfer_fragment fragment;
> + u8 data[] ____cacheline_aligned; /* variable length payload here */
> +} __packed;
> +
> +struct loader_xfer_dma_fragment {
> + struct loader_xfer_fragment fragment;
> + u64 ddr_phys_addr;
> +} __packed;
> +
> +struct loader_start {
> + struct loader_msg_hdr hdr;
> +} __packed;
> +
> +/**
> + * struct response_info - Encapsulate firmware response related
> + * information for passing between function
> + * loader_cl_send() and process_recv() callback.
> + * @data Copy the data received from firmware here.
> + * @max_size Max size allocated for the receive buffer
> + * @size Size of data received from firmware.
> + * @error Returns 0 for success, negative error code for a
> + * failure in function process_recv().
> + * flag_response Set to true on receiving a valid firmware
> + * response to host command
> + * wait_queue Wait queue for Host firmware loading where the
> + * client sends message to ISH firmware and waits
> + * for response
> + */
> +struct response_info {
> + void *data;
> + size_t max_size;
> + size_t size;
> + int error;
> + bool flag_response;
> + wait_queue_head_t wait_queue;
> +};
> +
> +/**
> + * struct ishtp_cl_data - Encapsulate per ISH-TP Client Data.
> + * @work_ishtp_reset: Work queue for reset handling.
> + * @work_fw_load: Work queue for host firmware loading.
> + * @flag_retry Flag for indicating host firmware loading should
> + * be retried.
> + * @retry_count Count the number of retries.
> + *
> + * This structure is used to store data per client.
> + */
> +struct ishtp_cl_data {
> + struct ishtp_cl *loader_ishtp_cl;
> + struct ishtp_cl_device *cl_device;
> +
> + /*
> + * Used for passing firmware response information between
> + * loader_cl_send() and process_recv() callback.
> + */
> + struct response_info response;
> +
> + struct work_struct work_ishtp_reset;
> + struct work_struct work_fw_load;
> +
> + /*
> + * In certain failure scenrios, it makes sense to reset the ISH
> + * subsystem and retry Host firmware loading (e.g. bad message
> + * packet, ENOMEM, etc.). On the other hand, failures due to
> + * protocol mismatch, etc., are not recoverable. We do not
> + * retry them.
> + *
> + * If set, the flag indicates that we should re-try the
> + * particular failure.
> + */
> + bool flag_retry;
> + int retry_count;
> +};
> +
> +#define IPC_FRAGMENT_DATA_PREAMBLE \
> + offsetof(struct loader_xfer_ipc_fragment, data)
> +
> +#define cl_data_to_dev(client_data) ishtp_device((client_data)->cl_device)
> +
> +/**
> + * get_firmware_variant() - Gets the filename of firmware image to be
> + * loaded based on platform variant.
> + * @client_data Client data instance.
> + * @filename Returns firmware filename.
> + *
> + * Queries the firmware-name device property string.
> + *
> + * Return: 0 for success, negative error code for failure.
> + */
> +static int get_firmware_variant(struct ishtp_cl_data *client_data,
> + char *filename)
> +{
> + int rv;
> + const char *val;
> + struct device *devc = ishtp_get_pci_device(client_data->cl_device);
> +
> + rv = device_property_read_string(devc, "firmware-name", &val);
> + if (rv < 0) {
> + dev_err(devc,
> + "Error: ISH firmware-name device property required\n");
> + return rv;
> + }
> + return snprintf(filename, FILENAME_SIZE, "intel/%s", val);
> +}
> +
> +/**
> + * loader_cl_send() Send message from host to firmware
> + * @client_data: Client data instance
> + * @out_msg Message buffer to be sent to firmware
> + * @out_size Size of out going message
> + * @in_msg Message buffer where the incoming data copied.
> + * This buffer is allocated by calling
> + * @in_size Max size of incoming message
> + *
> + * Return: Received buffer size on success, negative error code on failure.
Number of bytes copied to in_msg? Maybe a bit different than number of
bytes received?
> + */
> +static int loader_cl_send(struct ishtp_cl_data *client_data,
> + u8 *out_msg, size_t out_size,
> + u8 *in_msg, size_t in_size)
> +{
> + int rv;
> + struct loader_msg_hdr *in_hdr;
> + struct loader_msg_hdr *out_hdr = (struct loader_msg_hdr *)out_msg;
> + struct ishtp_cl *loader_ishtp_cl = client_data->loader_ishtp_cl;
> +
> + dev_dbg(cl_data_to_dev(client_data),
> + "%s: command=%02lx is_response=%u status=%02x\n",
> + __func__,
> + out_hdr->command & CMD_MASK,
> + out_hdr->command & IS_RESPONSE ? 1 : 0,
> + out_hdr->status);
> +
> + /* Setup in coming buffer & size */
> + client_data->response.data = in_msg;
> + client_data->response.max_size = in_size;
> + client_data->response.error = 0;
> + client_data->response.flag_response = false;
nit:
Consider renaming to something a bit more descriptive
such as "response.received"?
> +
> + rv = ishtp_cl_send(loader_ishtp_cl, out_msg, out_size);
> + if (rv < 0) {
> + dev_err(cl_data_to_dev(client_data),
> + "ishtp_cl_send error %d\n", rv);
> + return rv;
> + }
> +
> + wait_event_interruptible_timeout(client_data->response.wait_queue,
> + client_data->response.flag_response,
> + ISHTP_SEND_TIMEOUT);
> + if (!client_data->response.flag_response) {
> + dev_err(cl_data_to_dev(client_data),
> + "Timed out for response to command=%02lx",
> + out_hdr->command & CMD_MASK);
> + return -ETIMEDOUT;
> + }
> +
> + if (client_data->response.error < 0)
> + return client_data->response.error;
> +
> + /* All response messages will contain a header */
> + in_hdr = (struct loader_msg_hdr *)in_msg;
> +
> + /* Sanity checks */
> + if (!(in_hdr->command & IS_RESPONSE)) {
> + dev_err(cl_data_to_dev(client_data),
> + "Invalid response to command\n");
> + return -EIO;
> + }
> +
> + if (in_hdr->status) {
> + dev_err(cl_data_to_dev(client_data),
> + "Loader returned status %d\n",
> + in_hdr->status);
> + return -EIO;
> + }
There are two places these sanity checks happen: Here and in
process_recv(). I think they should all be in the same place. Before,
I said move them into here (since then we could return error codes),
but now that you added client_data->response.error, it seems
equivalent to move these down there. So I would put these two
checks down there, then you don't have to cast in_msg at all.
There is new context, but sorry to flip flop!
> +
> + return client_data->response.size;
> +}
> +
> +/**
> + * process_recv() - Receive and parse incoming packet
> + * @loader_ishtp_cl: Client instance to get stats
> + * @rb_in_proc: ISH received message buffer
> + *
> + * Parse the incoming packet. If it is a response packet then it will
> + * update flag_response and wake up the caller waiting to for the response.
> + */
> +static void process_recv(struct ishtp_cl *loader_ishtp_cl,
> + struct ishtp_cl_rb *rb_in_proc)
> +{
> + struct loader_msg_hdr *hdr;
> + size_t data_len = rb_in_proc->buf_idx;
> + struct ishtp_cl_data *client_data =
> + ishtp_get_client_data(loader_ishtp_cl);
> +
> + /*
> + * All firmware messages have a header. Check buffer size
> + * before accessing elements inside.
> + */
> + if (!rb_in_proc->buffer.data) {
> + dev_warn(cl_data_to_dev(client_data),
> + "rb_in_proc->buffer.data returned null");
> + client_data->response.error = -EBADMSG;
> + goto end_error;
> + }
> + if (data_len < sizeof(struct loader_msg_hdr)) {
> + dev_err(cl_data_to_dev(client_data),
> + "data size %zu is less than header %zu\n",
> + data_len, sizeof(struct loader_msg_hdr));
> + client_data->response.error = -EMSGSIZE;
> + goto end_error;
> + }
> +
> + hdr = (struct loader_msg_hdr *)rb_in_proc->buffer.data;
> +
> + dev_dbg(cl_data_to_dev(client_data),
> + "%s: command=%02lx is_response=%u status=%02x\n",
> + __func__,
> + hdr->command & CMD_MASK,
> + hdr->command & IS_RESPONSE ? 1 : 0,
> + hdr->status);
> +
> + switch (hdr->command & CMD_MASK) {
> + case LOADER_CMD_XFER_QUERY:
> + case LOADER_CMD_XFER_FRAGMENT:
> + case LOADER_CMD_START:
> + /* Sanity check */
> + if (client_data->response.flag_response) {
> + dev_err(cl_data_to_dev(client_data),
> + "Previous firmware message not yet processed\n");
> + client_data->response.error = -EINVAL;
> + break;
> + }
> + if (!client_data->response.data) {
> + dev_err(cl_data_to_dev(client_data),
> + "Receiving buffer is null. Should be allocated by calling function\n");
> + client_data->response.error = -EINVAL;
> + break;
> + }
Here you are forcing the callers of loader_cl_send() to allocate some data,
but in multiple cases this isn't necessary. Maybe you considered this,
but perhaps
you could make it so if this is NULL then just don't copy any data,
but still succeed?
Then callers of loader_cl_send() can just use NULL as in_msg if they don't want
the returned data. Update docstring of loader_cl_send() if so.
> +
> + if (data_len > client_data->response.max_size) {
> + dev_err(cl_data_to_dev(client_data),
> + "Received buffer size %zu is larger than allocated buffer %zu\n",
> + data_len, client_data->response.max_size);
> + client_data->response.error = -EMSGSIZE;
> + break;
> + }
Here you need to decide the meaning of the in_size arg for
loader_cl_send(): Does it mean
"The required size of incoming data" or "max size to copy to in_data"?
One is more strict, which
could be good for error checking, but the second is more in line to
the read() syscall and some
other read() functions. At the least fix the docstring of
loader_cl_send(). I think I'm fine with either,
maybe someone else has an opinion?
This could be the alternative algorithm:
bytes_to_copy = min(data_len, client_data->response.max_size);
client_data->response.size = bytes_to_copy;
if (bytes_to_copy && !client_data->response.data)
error()
memcpy(client_data->response.data, b_in_proc->buffer.data, bytes_to_copy);
> +
> + /* Update the actual received buffer size */
> + client_data->response.size = data_len;
> +
> + /*
> + * Copy the buffer received in firmware response for the
> + * calling thread.
> + */
> + memcpy(client_data->response.data,
> + rb_in_proc->buffer.data, data_len);
> +
> + /* Free the buffer */
> + ishtp_cl_io_rb_recycle(rb_in_proc);
> + rb_in_proc = NULL;
> +
> + /* Wake the calling thread */
> + client_data->response.flag_response = true;
> + wake_up_interruptible(&client_data->response.wait_queue);
> + break;
> +
> + default:
> + dev_err(cl_data_to_dev(client_data),
> + "Invalid command=%02lx\n",
> + hdr->command & CMD_MASK);
> + client_data->response.error = -EPROTO;
> + }
Instead of placing the "normal" code inside the successful cases of the switch
statement, move the "normal" code out here:
https://refactoring.com/catalog/replaceNestedConditionalWithGuardClauses.html
Using these guard clauses has two benefits:
-reduces indenting and visual complexity
-separates main logic from error checking
> +
> +end_error:
> + /* Free the buffer if we did not do above */
> + if (rb_in_proc)
> + ishtp_cl_io_rb_recycle(rb_in_proc);
> +}
> +
> +/**
> + * loader_cl_event_cb() - bus driver callback for incoming message
> + * @device: Pointer to the ishtp client device for which this
> + * message is targeted
> + *
> + * Remove the packet from the list and process the message by calling
> + * process_recv
> + */
> +static void loader_cl_event_cb(struct ishtp_cl_device *cl_device)
> +{
> + struct ishtp_cl_rb *rb_in_proc;
> + struct ishtp_cl_data *client_data;
> + struct ishtp_cl *loader_ishtp_cl = ishtp_get_drvdata(cl_device);
> +
> + client_data = ishtp_get_client_data(loader_ishtp_cl);
You never use client_data, remove it?
Or you could change process_recv() to use client_data as argument, since that is
sufficient for it.
> +
> + while ((rb_in_proc = ishtp_cl_rx_get_rb(loader_ishtp_cl)) != NULL) {
> + /* Process the data packet from firmware */
> + process_recv(loader_ishtp_cl, rb_in_proc);
> + }
> +}
> +
> +/**
> + * ish_query_loader_prop() - Query ISH Shim firmware loader
> + * @client_data: Client data instance
> + * @fw: Poiner to firmware data struct in host memory
> + *
> + * This function queries the ISH Shim firmware loader for capabilities.
> + *
> + * Return: 0 for success, negative error code for failure.
> + */
> +static int ish_query_loader_prop(struct ishtp_cl_data *client_data,
> + const struct firmware *fw,
> + struct shim_fw_info *fw_info)
> +{
> + int rv;
> + struct loader_xfer_query ldr_xfer_query;
> + struct loader_xfer_query_response ldr_xfer_query_resp;
> +
> + memset(&ldr_xfer_query, 0, sizeof(ldr_xfer_query));
> + ldr_xfer_query.hdr.command = LOADER_CMD_XFER_QUERY;
> + ldr_xfer_query.image_size = fw->size;
> + rv = loader_cl_send(client_data,
> + (u8 *)&ldr_xfer_query,
> + sizeof(ldr_xfer_query),
> + (u8 *)&ldr_xfer_query_resp,
> + sizeof(ldr_xfer_query_resp));
> + if (rv < 0) {
> + client_data->flag_retry = true;
> + return rv;
> + }
> +
> + /* On success, the return value is the received buffer size */
> + if (rv != sizeof(struct loader_xfer_query_response)) {
> + dev_err(cl_data_to_dev(client_data),
> + "data size %d is not equal to size of loader_xfer_query_response %zu\n",
> + rv, sizeof(struct loader_xfer_query_response));
> + client_data->flag_retry = true;
> + return -EMSGSIZE;
> + }
> +
> + /* Save fw_info for use outside this function */
> + *fw_info = ldr_xfer_query_resp.fw_info;
> +
> + /* Loader firmware properties */
> + dev_dbg(cl_data_to_dev(client_data),
> + "ish_fw_version: major=%d minor=%d hotfix=%d build=%d protocol_version=0x%x loader_version=%d\n",
> + fw_info->ish_fw_version.major,
> + fw_info->ish_fw_version.minor,
> + fw_info->ish_fw_version.hotfix,
> + fw_info->ish_fw_version.build,
> + fw_info->protocol_version,
> + fw_info->ldr_version.value);
> +
> + dev_dbg(cl_data_to_dev(client_data),
> + "loader_capability: max_fw_image_size=0x%x xfer_mode=%d max_dma_buf_size=0x%x dma_buf_size_limit=0x%x\n",
> + fw_info->ldr_capability.max_fw_image_size,
> + fw_info->ldr_capability.xfer_mode,
> + fw_info->ldr_capability.max_dma_buf_size,
> + dma_buf_size_limit);
> +
> + /* Sanity checks */
> + if (fw_info->ldr_capability.max_fw_image_size < fw->size) {
> + dev_err(cl_data_to_dev(client_data),
> + "ISH firmware size %zu is greater than Shim firmware loader max supported %d\n",
> + fw->size,
> + fw_info->ldr_capability.max_fw_image_size);
> + return -ENOSPC;
> + }
> +
> + /* For DMA the buffer size should be multiple of cacheline size */
> + if ((fw_info->ldr_capability.xfer_mode & LOADER_XFER_MODE_DIRECT_DMA) &&
> + (fw_info->ldr_capability.max_dma_buf_size % L1_CACHE_BYTES)) {
> + dev_err(cl_data_to_dev(client_data),
> + "Shim firmware loader buffer size %d should be multipe of cacheline\n",
> + fw_info->ldr_capability.max_dma_buf_size);
> + return -EINVAL;
> + }
> +
> + return 0;
> +}
> +
> +/**
> + * 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) {
> + client_data->flag_retry = true;
> + return -ENOMEM;
> + }
> +
> + ldr_xfer_ipc_frag->fragment.hdr.command = LOADER_CMD_XFER_FRAGMENT;
> + ldr_xfer_ipc_frag->fragment.xfer_mode = LOADER_XFER_MODE_ISHTP;
> +
> + /* Break the firmware image into fragments and send as ISH-TP payload */
> + fragment_offset = 0;
> + while (fragment_offset < fw->size) {
> + if (fragment_offset + payload_max_size < fw->size) {
> + fragment_size = payload_max_size;
> + ldr_xfer_ipc_frag->fragment.is_last = 0;
> + } else {
> + fragment_size = fw->size - fragment_offset;
> + ldr_xfer_ipc_frag->fragment.is_last = 1;
> + }
> +
> + ldr_xfer_ipc_frag->fragment.offset = fragment_offset;
> + ldr_xfer_ipc_frag->fragment.size = fragment_size;
> + memcpy(ldr_xfer_ipc_frag->data,
> + &fw->data[fragment_offset],
> + fragment_size);
> +
> + dev_dbg(cl_data_to_dev(client_data),
> + "xfer_mode=ipc offset=0x%08x size=0x%08x is_last=%d\n",
> + ldr_xfer_ipc_frag->fragment.offset,
> + ldr_xfer_ipc_frag->fragment.size,
> + ldr_xfer_ipc_frag->fragment.is_last);
> +
> + rv = loader_cl_send(client_data,
> + (u8 *)ldr_xfer_ipc_frag,
> + IPC_FRAGMENT_DATA_PREAMBLE + fragment_size,
> + (u8 *)&ldr_xfer_ipc_ack,
> + sizeof(ldr_xfer_ipc_ack));
> + if (rv < 0) {
> + client_data->flag_retry = true;
> + goto end_err_resp_buf_release;
> + }
> +
> + fragment_offset += fragment_size;
> + }
> +
> + kfree(ldr_xfer_ipc_frag);
> + return 0;
> +
> +end_err_resp_buf_release:
> + /* Free ISH buffer if not done already, in error case */
> + kfree(ldr_xfer_ipc_frag);
> + return rv;
> +}
> +
> +/**
> + * ish_fw_xfer_direct_dma() - Loads ISH firmware using direct dma
> + * @client_data: Client data instance
> + * @fw: Poiner to firmware data struct in host memory
add @fw_info. And there's still a typo above. and make fw_info const below?
> + *
> + * Host firmware load is a unique case where we need to download
> + * a large firmware image (200+ Kb). This function implements
> + * direct DMA transfer in kernel and ISH firmware. This allows
> + * us to overcome the ISH-TP 4 Kb limit, and allows us to DMA
> + * directly to ISH UMA at location of choice.
> + * Function depends on corresponding support in ISH firmware.
> + *
> + * Return: 0 for success, negative error code for failure.
> + */
> +static int ish_fw_xfer_direct_dma(struct ishtp_cl_data *client_data,
> + const struct firmware *fw,
> + struct shim_fw_info fw_info)
> +{
> + int rv;
> + void *dma_buf;
> + dma_addr_t dma_buf_phy;
> + u32 fragment_offset, fragment_size, payload_max_size;
> + struct loader_msg_hdr ldr_xfer_dma_frag_ack;
> + struct loader_xfer_dma_fragment ldr_xfer_dma_frag;
> + struct device *devc = ishtp_get_pci_device(client_data->cl_device);
> + u32 shim_fw_buf_size =
> + fw_info.ldr_capability.max_dma_buf_size;
> +
> + /*
> + * 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) {
> + 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;
> +
> + /* Send the firmware image in chucks of payload_max_size */
> + fragment_offset = 0;
> + while (fragment_offset < fw->size) {
> + if (fragment_offset + payload_max_size < fw->size) {
> + fragment_size = payload_max_size;
> + ldr_xfer_dma_frag.fragment.is_last = 0;
> + } else {
> + fragment_size = fw->size - fragment_offset;
> + ldr_xfer_dma_frag.fragment.is_last = 1;
> + }
> +
> + ldr_xfer_dma_frag.fragment.offset = fragment_offset;
> + 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);
> +
> + /*
> + * Flush cache here because the dma_sync_single_for_device()
> + * does not do for x86.
> + */
> + clflush_cache_range(dma_buf, payload_max_size);
> +
> + dev_dbg(cl_data_to_dev(client_data),
> + "xfer_mode=dma offset=0x%08x size=0x%x is_last=%d ddr_phys_addr=0x%016llx\n",
> + ldr_xfer_dma_frag.fragment.offset,
> + ldr_xfer_dma_frag.fragment.size,
> + ldr_xfer_dma_frag.fragment.is_last,
> + ldr_xfer_dma_frag.ddr_phys_addr);
> +
> + rv = loader_cl_send(client_data,
> + (u8 *)&ldr_xfer_dma_frag,
> + sizeof(ldr_xfer_dma_frag),
> + (u8 *)&ldr_xfer_dma_frag_ack,
> + sizeof(ldr_xfer_dma_frag_ack));
> + if (rv < 0) {
> + client_data->flag_retry = true;
> + goto end_err_resp_buf_release;
> + }
> +
> + 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);
> + return rv;
> +}
> +
> +/**
> + * ish_fw_start() Start executing ISH main firmware
> + * @client_data: client data instance
> + *
> + * This function sends message to Shim firmware loader to start
> + * the execution of ISH main firmware.
> + *
> + * Return: 0 for success, negative error code for failure.
> + */
> +static int ish_fw_start(struct ishtp_cl_data *client_data)
> +{
> + int rv;
> + struct loader_start ldr_start;
> + struct loader_msg_hdr ldr_start_ack;
> +
> + memset(&ldr_start, 0, sizeof(ldr_start));
> + ldr_start.hdr.command = LOADER_CMD_START;
> + rv = loader_cl_send(client_data,
> + (u8 *)&ldr_start,
> + sizeof(ldr_start),
> + (u8 *)&ldr_start_ack,
> + sizeof(ldr_start_ack));
> +
> + return rv;
Remove rv and just return loader_cl_send()
> +}
> +
> +/**
> + * load_fw_from_host() Loads ISH firmware from host
> + * @client_data: Client data instance
> + *
> + * This function loads the ISH firmware to ISH SRAM and starts execution
> + *
> + * Return: 0 for success, negative error code for failure.
> + */
> +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) {
> + rv = -ENOMEM;
> + goto end_error;
flag_retry is false here, so a re-attempt will not happen, is that OK?
> + }
> +
> + /* Get filename of the ISH firmware to be loaded */
> + rv = get_firmware_variant(client_data, filename);
> + if (rv < 0)
> + goto end_err_filename_buf_release;
> +
> + rv = request_firmware(&fw, filename, cl_data_to_dev(client_data));
> + if (rv < 0)
> + goto end_err_filename_buf_release;
> +
> + /* Step 1: Query Shim firmware loader properties */
> +
> + rv = ish_query_loader_prop(client_data, fw, &fw_info);
> + if (rv < 0)
> + goto end_err_fw_release;
> +
> + /* Step 2: Send the main firmware image to be loaded, to ISH SRAM */
> +
> + xfer_mode = fw_info.ldr_capability.xfer_mode;
> + if (xfer_mode & LOADER_XFER_MODE_DIRECT_DMA) {
> + rv = ish_fw_xfer_direct_dma(client_data, fw, fw_info);
> + } else if (xfer_mode & LOADER_XFER_MODE_ISHTP) {
> + rv = ish_fw_xfer_ishtp(client_data, fw);
> + } else {
> + dev_err(cl_data_to_dev(client_data),
> + "No transfer mode selected in firmware\n");
> + rv = -EINVAL;
> + }
> + if (rv < 0)
> + goto end_err_fw_release;
> +
> + /* Step 3: Start ISH main firmware exeuction */
> +
> + rv = ish_fw_start(client_data);
> + if (rv < 0)
> + goto end_err_fw_release;
> +
> + release_firmware(fw);
> + kfree(filename);
> + dev_info(cl_data_to_dev(client_data), "ISH firmware %s loaded\n",
> + filename);
> + return 0;
> +
> +end_err_fw_release:
> + release_firmware(fw);
> +end_err_filename_buf_release:
> + kfree(filename);
> +end_error:
> + /* Keep a count of retries, and give up after 3 attempts */
> + if (client_data->flag_retry && client_data->retry_count++ < 3) {
#define MAX_LOAD_ATTEMPTS at the top of the file?
Also consider explicitly initializing retry count to 0?
> + dev_warn(cl_data_to_dev(client_data),
> + "ISH host firmware load failed %d. Reset ISH & try again..\n",
> + rv);
Maybe change Reset to Resetting? Reset sounds like it is requesting the
user to perform an action.
> + ish_hw_reset(ishtp_get_ishtp_device(loader_ishtp_cl));
> + } else {
> + dev_err(cl_data_to_dev(client_data),
> + "ISH host firmware load failed %d\n", rv);
> + }
> + return rv;
> +}
> +
> +static void load_fw_from_host_handler(struct work_struct *work)
> +{
> + struct ishtp_cl_data *client_data;
> +
> + client_data = container_of(work, struct ishtp_cl_data,
> + work_fw_load);
> + load_fw_from_host(client_data);
If load_fw_from_host() fails then maybe just log the error code?
> +}
> +
> +/**
> + * loader_init() - Init function for ISH-TP client
> + * @loader_ishtp_cl: ISH-TP client instance
> + * @reset: true if called for init after reset
> + *
> + * Return: 0 for success, negative error code for failure
> + */
> +static int loader_init(struct ishtp_cl *loader_ishtp_cl, int reset)
> +{
> + int rv;
> + struct ishtp_fw_client *fw_client;
> + struct ishtp_cl_data *client_data =
> + ishtp_get_client_data(loader_ishtp_cl);
> +
> + dev_dbg(cl_data_to_dev(client_data), "reset flag: %d\n", reset);
> +
> + rv = ishtp_cl_link(loader_ishtp_cl);
> + if (rv < 0) {
> + dev_err(cl_data_to_dev(client_data), "ishtp_cl_link failed\n");
> + return rv;
> + }
> +
> + /* Connect to firmware client */
> + ishtp_set_tx_ring_size(loader_ishtp_cl, LOADER_CL_TX_RING_SIZE);
> + ishtp_set_rx_ring_size(loader_ishtp_cl, LOADER_CL_RX_RING_SIZE);
> +
> + fw_client =
> + ishtp_fw_cl_get_client(ishtp_get_ishtp_device(loader_ishtp_cl),
> + &loader_ishtp_guid);
> + if (!fw_client) {
> + dev_err(cl_data_to_dev(client_data),
> + "ISH client uuid not found\n");
> + rv = -ENOENT;
> + goto err_cl_unlink;
> + }
> +
> + ishtp_cl_set_fw_client_id(loader_ishtp_cl,
> + ishtp_get_fw_client_id(fw_client));
> + ishtp_set_connection_state(loader_ishtp_cl, ISHTP_CL_CONNECTING);
> +
> + rv = ishtp_cl_connect(loader_ishtp_cl);
> + if (rv < 0) {
> + dev_err(cl_data_to_dev(client_data), "Client connect fail\n");
> + goto err_cl_unlink;
> + }
> +
> + dev_dbg(cl_data_to_dev(client_data), "Client connected\n");
> +
> + ishtp_register_event_cb(client_data->cl_device, loader_cl_event_cb);
> +
> + return 0;
> +
> +err_cl_unlink:
> + ishtp_cl_unlink(loader_ishtp_cl);
> + return rv;
> +}
> +
> +static void loader_deinit(struct ishtp_cl *loader_ishtp_cl)
> +{
> + ishtp_set_connection_state(loader_ishtp_cl, ISHTP_CL_DISCONNECTING);
> + ishtp_cl_disconnect(loader_ishtp_cl);
> + ishtp_cl_unlink(loader_ishtp_cl);
> + ishtp_cl_flush_queues(loader_ishtp_cl);
> +
> + /* Disband and free all Tx and Rx client-level rings */
> + ishtp_cl_free(loader_ishtp_cl);
> +}
> +
> +static void reset_handler(struct work_struct *work)
> +{
> + int rv;
> + struct ishtp_cl_data *client_data;
> + struct ishtp_cl *loader_ishtp_cl;
> + struct ishtp_cl_device *cl_device;
> +
> + client_data = container_of(work, struct ishtp_cl_data,
> + work_ishtp_reset);
> +
> + loader_ishtp_cl = client_data->loader_ishtp_cl;
> + cl_device = client_data->cl_device;
> +
> + /* Unlink, flush queues & start again */
> + ishtp_cl_unlink(loader_ishtp_cl);
> + ishtp_cl_flush_queues(loader_ishtp_cl);
> + ishtp_cl_free(loader_ishtp_cl);
> +
> + loader_ishtp_cl = ishtp_cl_allocate(cl_device);
> + if (!loader_ishtp_cl)
> + return;
> +
> + 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;
> +
> + rv = loader_init(loader_ishtp_cl, 1);
> + if (rv < 0) {
> + dev_err(ishtp_device(cl_device), "Reset Failed\n");
> + return;
> + }
> +
> + /* ISH firmware loading from host */
> + load_fw_from_host(client_data);
> +}
> +
> +/**
> + * 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)
> + return -ENOMEM;
> +
> + loader_ishtp_cl = ishtp_cl_allocate(cl_device);
> + if (!loader_ishtp_cl)
> + 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(&client_data->response.wait_queue);
> +
> + INIT_WORK(&client_data->work_ishtp_reset,
> + reset_handler);
> + INIT_WORK(&client_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);
> +
> + /* ISH firmware loading from host */
> + schedule_work(&client_data->work_fw_load);
> +
> + return 0;
> +}
> +
> +/**
> + * loader_ishtp_cl_remove() - ISH-TP client driver remove
> + * @cl_device: ISH-TP client device instance
> + *
> + * This function gets called on device remove on ISH-TP bus
> + *
> + * Return: 0
> + */
> +static int loader_ishtp_cl_remove(struct ishtp_cl_device *cl_device)
> +{
> + struct ishtp_cl_data *client_data;
> + struct ishtp_cl *loader_ishtp_cl = ishtp_get_drvdata(cl_device);
> +
> + client_data = ishtp_get_client_data(loader_ishtp_cl);
> +
> + /*
> + * The sequence of the following two cancel_work_sync() is
> + * important. The work_fw_load can in turn schedue
> + * work_ishtp_reset, so first cancel work_fw_load then
> + * cancel work_ishtp_reset.
> + */
> + cancel_work_sync(&client_data->work_fw_load);
> + cancel_work_sync(&client_data->work_ishtp_reset);
> + loader_deinit(loader_ishtp_cl);
> + ishtp_put_device(cl_device);
> +
> + return 0;
> +}
> +
> +/**
> + * loader_ishtp_cl_reset() - ISH-TP client driver reset
> + * @cl_device: ISH-TP client device instance
> + *
> + * This function gets called on device reset on ISH-TP bus
> + *
> + * Return: 0
> + */
> +static int loader_ishtp_cl_reset(struct ishtp_cl_device *cl_device)
> +{
> + struct ishtp_cl_data *client_data;
> + struct ishtp_cl *loader_ishtp_cl = ishtp_get_drvdata(cl_device);
> +
> + client_data = ishtp_get_client_data(loader_ishtp_cl);
> +
> + schedule_work(&client_data->work_ishtp_reset);
> +
> + return 0;
> +}
> +
> +static struct ishtp_cl_driver loader_ishtp_cl_driver = {
> + .name = "ish-loader",
> + .guid = &loader_ishtp_guid,
> + .probe = loader_ishtp_cl_probe,
> + .remove = loader_ishtp_cl_remove,
> + .reset = loader_ishtp_cl_reset,
> +};
> +
> +static int __init ish_loader_init(void)
> +{
> + return ishtp_cl_driver_register(&loader_ishtp_cl_driver, THIS_MODULE);
> +}
> +
> +static void __exit ish_loader_exit(void)
> +{
> + ishtp_cl_driver_unregister(&loader_ishtp_cl_driver);
> +}
> +
> +late_initcall(ish_loader_init);
> +module_exit(ish_loader_exit);
> +
> +module_param(dma_buf_size_limit, int, 0644);
> +MODULE_PARM_DESC(dma_buf_size_limit, "Limit the DMA buf size to this value in bytes");
> +
> +MODULE_DESCRIPTION("ISH ISH-TP Host firmware Loader Client Driver");
> +MODULE_AUTHOR("Rushikesh S Kadam <rushikesh.s.kadam@intel.com>");
> +
> +MODULE_LICENSE("GPL v2");
> +MODULE_ALIAS("ishtp:*");
> --
> 1.9.1
>
After those final tweaks, then oh my goodness,
Acked-by: Nick Crews <ncrews@chromium.org>
^ permalink raw reply
* RE: [PATCH v4] HID: core: move Usage Page concatenation to Main item
From: Junge, Terry @ 2019-03-29 0:48 UTC (permalink / raw)
To: Benjamin Tissoires
Cc: Nicolas Saenz Julienne, Jiri Kosina, oneukum@suse.de,
linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <CAO-hwJLatVKwgevfdFk1Lqqpg3H7Oc28_VEkiyxPXne3fhSrqw@mail.gmail.com>
On Thursday, March 28, 2019 6:49 AM Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote:
>>
>> Hi Nicolas,
>>
>> V4 looks good to me.
>
>Looks good to me too.
>
>Terry, can I consider this as a formal Rev-by you?
Benjamin,
Yes, thank you. What is the normal way of formally indicating Rev-by?
Regards,
Terry
>
>Cheers,
>Benjamin
>
>>
>> Thanks,
>> Terry
>>
>> >-----Original Message-----
>> >Subject: [PATCH v4] HID: core: move Usage Page concatenation to Main
>item
>> >
^ permalink raw reply
* [PATCH v2] HID: intel-ish-hid: ISH firmware loader client driver
From: Rushikesh S Kadam @ 2019-03-28 20:20 UTC (permalink / raw)
To: srinivas.pandruvada, benjamin.tissoires, jikos
Cc: ncrews, jettrink, gwendal, rushikesh.s.kadam, linux-kernel,
linux-input
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 <rushikesh.s.kadam@intel.com>
---
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
The v2 revision primarily address review comments received on
the v1 patch.
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 | 1084 +++++++++++++++++++++++++++
4 files changed, 1103 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 0000000..8685fa6
--- /dev/null
+++ b/drivers/hid/intel-ish-hid/ishtp-fw-loader.c
@@ -0,0 +1,1084 @@
+// SPDX-License-Identifier: GPL-2.0
+/*
+ * ISH-TP client driver for ISH firmware loading
+ *
+ * Copyright (c) 2019, Intel Corporation.
+ */
+
+#include <linux/firmware.h>
+#include <linux/module.h>
+#include <linux/pci.h>
+#include <linux/intel-ish-client-if.h>
+#include <linux/property.h>
+#include <asm/cacheflush.h>
+
+/* 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_FRAGMENT Transfer one firmware image fragment at a
+ * time. The command may be executed
+ * multiple times until the entire firmware
+ * image is downloaded to SRAM.
+ * LOADER_CMD_START Start executing the main firmware.
+ */
+enum ish_loader_commands {
+ LOADER_CMD_XFER_QUERY = 0,
+ LOADER_CMD_XFER_FRAGMENT,
+ LOADER_CMD_START,
+};
+
+/* Command bit mask */
+#define CMD_MASK GENMASK(6, 0)
+#define IS_RESPONSE BIT(7)
+
+/*
+ * ISH firmware max delay for one transmit failure is 1 Hz,
+ * and firmware will retry 2 times, so 3 Hz is used for timeout.
+ */
+#define ISHTP_SEND_TIMEOUT (3 * HZ)
+
+/*
+ * Loader transfer modes:
+ *
+ * LOADER_XFER_MODE_ISHTP mode uses the existing ISH-TP mechanism to
+ * transfer data. This may use IPC or DMA if supported in firmware.
+ * The buffer size is limited to 4 Kb by the IPC/ISH-TP protocol for
+ * both IPC & DMA (legacy).
+ *
+ * LOADER_XFER_MODE_DIRECT_DMA - firmware loading is a bit different
+ * from the sensor data streaming. Here we download a large (300+ Kb)
+ * image directly to ISH SRAM memory. There is limited benefit of
+ * DMA'ing 300 Kb image in 4 Kb chucks limit. Hence, we introduce
+ * this "direct dma" mode, where we do not use ISH-TP for DMA, but
+ * instead manage the DMA directly in kernel driver and Shim firmware
+ * loader (allocate buffer, break in chucks and transfer). This allows
+ * to overcome 4 Kb limit, and optimize the data flow path in firmware.
+ */
+#define LOADER_XFER_MODE_DIRECT_DMA BIT(0)
+#define LOADER_XFER_MODE_ISHTP BIT(1)
+
+/* ISH Transport Loader client unique GUID */
+static const guid_t loader_ishtp_guid =
+ GUID_INIT(0xc804d06a, 0x55bd, 0x4ea7,
+ 0xad, 0xed, 0x1e, 0x31, 0x22, 0x8c, 0x76, 0xdc);
+
+#define FILENAME_SIZE 256
+
+/*
+ * The firmware loading latency will be minimum if we can DMA the
+ * entire ISH firmware image in one go. This requires that we allocate
+ * a large DMA buffer in kernel, which could be problematic on some
+ * platforms. So here we limit the DMA buffer size via a module_param.
+ * We default to 4 pages, but a customer can set it to higher limit if
+ * deemed appropriate for his platform.
+ */
+static int dma_buf_size_limit = 4 * PAGE_SIZE;
+
+/**
+ * struct loader_msg_hdr - Header for ISH Loader commands.
+ * @command: LOADER_CMD* commands. Bit 7 is the response.
+ * @status: Command response status. Non 0, is error
+ * condition.
+ *
+ * This structure is used as header for every command/data sent/received
+ * between Host driver and ISH Shim firmware loader.
+ */
+struct loader_msg_hdr {
+ u8 command;
+ u8 reserved[2];
+ u8 status;
+} __packed;
+
+struct loader_xfer_query {
+ struct loader_msg_hdr hdr;
+ u32 image_size;
+} __packed;
+
+struct ish_fw_version {
+ u16 major;
+ u16 minor;
+ u16 hotfix;
+ u16 build;
+} __packed;
+
+union loader_version {
+ u32 value;
+ struct {
+ u8 major;
+ u8 minor;
+ u8 hotfix;
+ u8 build;
+ };
+} __packed;
+
+struct loader_capability {
+ u32 max_fw_image_size;
+ u32 xfer_mode;
+ u32 max_dma_buf_size; /* only for dma mode, multiples of cacheline */
+} __packed;
+
+struct shim_fw_info {
+ struct ish_fw_version ish_fw_version;
+ u32 protocol_version;
+ union loader_version ldr_version;
+ struct loader_capability ldr_capability;
+} __packed;
+
+struct loader_xfer_query_response {
+ struct loader_msg_hdr hdr;
+ struct shim_fw_info fw_info;
+} __packed;
+
+struct loader_xfer_fragment {
+ struct loader_msg_hdr hdr;
+ u32 xfer_mode;
+ u32 offset;
+ u32 size;
+ u32 is_last;
+} __packed;
+
+struct loader_xfer_ipc_fragment {
+ struct loader_xfer_fragment fragment;
+ u8 data[] ____cacheline_aligned; /* variable length payload here */
+} __packed;
+
+struct loader_xfer_dma_fragment {
+ struct loader_xfer_fragment fragment;
+ u64 ddr_phys_addr;
+} __packed;
+
+struct loader_start {
+ struct loader_msg_hdr hdr;
+} __packed;
+
+/**
+ * struct response_info - Encapsulate firmware response related
+ * information for passing between function
+ * loader_cl_send() and process_recv() callback.
+ * @data Copy the data received from firmware here.
+ * @max_size Max size allocated for the receive buffer
+ * @size Size of data received from firmware.
+ * @error Returns 0 for success, negative error code for a
+ * failure in function process_recv().
+ * flag_response Set to true on receiving a valid firmware
+ * response to host command
+ * wait_queue Wait queue for Host firmware loading where the
+ * client sends message to ISH firmware and waits
+ * for response
+ */
+struct response_info {
+ void *data;
+ size_t max_size;
+ size_t size;
+ int error;
+ bool flag_response;
+ wait_queue_head_t wait_queue;
+};
+
+/**
+ * struct ishtp_cl_data - Encapsulate per ISH-TP Client Data.
+ * @work_ishtp_reset: Work queue for reset handling.
+ * @work_fw_load: Work queue for host firmware loading.
+ * @flag_retry Flag for indicating host firmware loading should
+ * be retried.
+ * @retry_count Count the number of retries.
+ *
+ * This structure is used to store data per client.
+ */
+struct ishtp_cl_data {
+ struct ishtp_cl *loader_ishtp_cl;
+ struct ishtp_cl_device *cl_device;
+
+ /*
+ * Used for passing firmware response information between
+ * loader_cl_send() and process_recv() callback.
+ */
+ struct response_info response;
+
+ struct work_struct work_ishtp_reset;
+ struct work_struct work_fw_load;
+
+ /*
+ * In certain failure scenrios, it makes sense to reset the ISH
+ * subsystem and retry Host firmware loading (e.g. bad message
+ * packet, ENOMEM, etc.). On the other hand, failures due to
+ * protocol mismatch, etc., are not recoverable. We do not
+ * retry them.
+ *
+ * If set, the flag indicates that we should re-try the
+ * particular failure.
+ */
+ bool flag_retry;
+ int retry_count;
+};
+
+#define IPC_FRAGMENT_DATA_PREAMBLE \
+ offsetof(struct loader_xfer_ipc_fragment, data)
+
+#define cl_data_to_dev(client_data) ishtp_device((client_data)->cl_device)
+
+/**
+ * get_firmware_variant() - Gets the filename of firmware image to be
+ * loaded based on platform variant.
+ * @client_data Client data instance.
+ * @filename Returns firmware filename.
+ *
+ * Queries the firmware-name device property string.
+ *
+ * Return: 0 for success, negative error code for failure.
+ */
+static int get_firmware_variant(struct ishtp_cl_data *client_data,
+ char *filename)
+{
+ int rv;
+ const char *val;
+ struct device *devc = ishtp_get_pci_device(client_data->cl_device);
+
+ rv = device_property_read_string(devc, "firmware-name", &val);
+ if (rv < 0) {
+ dev_err(devc,
+ "Error: ISH firmware-name device property required\n");
+ return rv;
+ }
+ return snprintf(filename, FILENAME_SIZE, "intel/%s", val);
+}
+
+/**
+ * loader_cl_send() Send message from host to firmware
+ * @client_data: Client data instance
+ * @out_msg Message buffer to be sent to firmware
+ * @out_size Size of out going message
+ * @in_msg Message buffer where the incoming data copied.
+ * This buffer is allocated by calling
+ * @in_size Max size of incoming message
+ *
+ * Return: Received buffer size on success, negative error code on failure.
+ */
+static int loader_cl_send(struct ishtp_cl_data *client_data,
+ u8 *out_msg, size_t out_size,
+ u8 *in_msg, size_t in_size)
+{
+ int rv;
+ struct loader_msg_hdr *in_hdr;
+ struct loader_msg_hdr *out_hdr = (struct loader_msg_hdr *)out_msg;
+ struct ishtp_cl *loader_ishtp_cl = client_data->loader_ishtp_cl;
+
+ dev_dbg(cl_data_to_dev(client_data),
+ "%s: command=%02lx is_response=%u status=%02x\n",
+ __func__,
+ out_hdr->command & CMD_MASK,
+ out_hdr->command & IS_RESPONSE ? 1 : 0,
+ out_hdr->status);
+
+ /* Setup in coming buffer & size */
+ client_data->response.data = in_msg;
+ client_data->response.max_size = in_size;
+ client_data->response.error = 0;
+ client_data->response.flag_response = false;
+
+ rv = ishtp_cl_send(loader_ishtp_cl, out_msg, out_size);
+ if (rv < 0) {
+ dev_err(cl_data_to_dev(client_data),
+ "ishtp_cl_send error %d\n", rv);
+ return rv;
+ }
+
+ wait_event_interruptible_timeout(client_data->response.wait_queue,
+ client_data->response.flag_response,
+ ISHTP_SEND_TIMEOUT);
+ if (!client_data->response.flag_response) {
+ dev_err(cl_data_to_dev(client_data),
+ "Timed out for response to command=%02lx",
+ out_hdr->command & CMD_MASK);
+ return -ETIMEDOUT;
+ }
+
+ if (client_data->response.error < 0)
+ return client_data->response.error;
+
+ /* All response messages will contain a header */
+ in_hdr = (struct loader_msg_hdr *)in_msg;
+
+ /* Sanity checks */
+ if (!(in_hdr->command & IS_RESPONSE)) {
+ dev_err(cl_data_to_dev(client_data),
+ "Invalid response to command\n");
+ return -EIO;
+ }
+
+ if (in_hdr->status) {
+ dev_err(cl_data_to_dev(client_data),
+ "Loader returned status %d\n",
+ in_hdr->status);
+ return -EIO;
+ }
+
+ return client_data->response.size;
+}
+
+/**
+ * process_recv() - Receive and parse incoming packet
+ * @loader_ishtp_cl: Client instance to get stats
+ * @rb_in_proc: ISH received message buffer
+ *
+ * Parse the incoming packet. If it is a response packet then it will
+ * update flag_response and wake up the caller waiting to for the response.
+ */
+static void process_recv(struct ishtp_cl *loader_ishtp_cl,
+ struct ishtp_cl_rb *rb_in_proc)
+{
+ struct loader_msg_hdr *hdr;
+ size_t data_len = rb_in_proc->buf_idx;
+ struct ishtp_cl_data *client_data =
+ ishtp_get_client_data(loader_ishtp_cl);
+
+ /*
+ * All firmware messages have a header. Check buffer size
+ * before accessing elements inside.
+ */
+ if (!rb_in_proc->buffer.data) {
+ dev_warn(cl_data_to_dev(client_data),
+ "rb_in_proc->buffer.data returned null");
+ client_data->response.error = -EBADMSG;
+ goto end_error;
+ }
+ if (data_len < sizeof(struct loader_msg_hdr)) {
+ dev_err(cl_data_to_dev(client_data),
+ "data size %zu is less than header %zu\n",
+ data_len, sizeof(struct loader_msg_hdr));
+ client_data->response.error = -EMSGSIZE;
+ goto end_error;
+ }
+
+ hdr = (struct loader_msg_hdr *)rb_in_proc->buffer.data;
+
+ dev_dbg(cl_data_to_dev(client_data),
+ "%s: command=%02lx is_response=%u status=%02x\n",
+ __func__,
+ hdr->command & CMD_MASK,
+ hdr->command & IS_RESPONSE ? 1 : 0,
+ hdr->status);
+
+ switch (hdr->command & CMD_MASK) {
+ case LOADER_CMD_XFER_QUERY:
+ case LOADER_CMD_XFER_FRAGMENT:
+ case LOADER_CMD_START:
+ /* Sanity check */
+ if (client_data->response.flag_response) {
+ dev_err(cl_data_to_dev(client_data),
+ "Previous firmware message not yet processed\n");
+ client_data->response.error = -EINVAL;
+ break;
+ }
+ if (!client_data->response.data) {
+ dev_err(cl_data_to_dev(client_data),
+ "Receiving buffer is null. Should be allocated by calling function\n");
+ client_data->response.error = -EINVAL;
+ break;
+ }
+
+ if (data_len > client_data->response.max_size) {
+ dev_err(cl_data_to_dev(client_data),
+ "Received buffer size %zu is larger than allocated buffer %zu\n",
+ data_len, client_data->response.max_size);
+ client_data->response.error = -EMSGSIZE;
+ break;
+ }
+
+ /* Update the actual received buffer size */
+ client_data->response.size = data_len;
+
+ /*
+ * Copy the buffer received in firmware response for the
+ * calling thread.
+ */
+ memcpy(client_data->response.data,
+ rb_in_proc->buffer.data, data_len);
+
+ /* Free the buffer */
+ ishtp_cl_io_rb_recycle(rb_in_proc);
+ rb_in_proc = NULL;
+
+ /* Wake the calling thread */
+ client_data->response.flag_response = true;
+ wake_up_interruptible(&client_data->response.wait_queue);
+ break;
+
+ default:
+ dev_err(cl_data_to_dev(client_data),
+ "Invalid command=%02lx\n",
+ hdr->command & CMD_MASK);
+ client_data->response.error = -EPROTO;
+ }
+
+end_error:
+ /* Free the buffer if we did not do above */
+ if (rb_in_proc)
+ ishtp_cl_io_rb_recycle(rb_in_proc);
+}
+
+/**
+ * loader_cl_event_cb() - bus driver callback for incoming message
+ * @device: Pointer to the ishtp client device for which this
+ * message is targeted
+ *
+ * Remove the packet from the list and process the message by calling
+ * process_recv
+ */
+static void loader_cl_event_cb(struct ishtp_cl_device *cl_device)
+{
+ struct ishtp_cl_rb *rb_in_proc;
+ struct ishtp_cl_data *client_data;
+ struct ishtp_cl *loader_ishtp_cl = ishtp_get_drvdata(cl_device);
+
+ client_data = ishtp_get_client_data(loader_ishtp_cl);
+
+ while ((rb_in_proc = ishtp_cl_rx_get_rb(loader_ishtp_cl)) != NULL) {
+ /* Process the data packet from firmware */
+ process_recv(loader_ishtp_cl, rb_in_proc);
+ }
+}
+
+/**
+ * ish_query_loader_prop() - Query ISH Shim firmware loader
+ * @client_data: Client data instance
+ * @fw: Poiner to firmware data struct in host memory
+ *
+ * This function queries the ISH Shim firmware loader for capabilities.
+ *
+ * Return: 0 for success, negative error code for failure.
+ */
+static int ish_query_loader_prop(struct ishtp_cl_data *client_data,
+ const struct firmware *fw,
+ struct shim_fw_info *fw_info)
+{
+ int rv;
+ struct loader_xfer_query ldr_xfer_query;
+ struct loader_xfer_query_response ldr_xfer_query_resp;
+
+ memset(&ldr_xfer_query, 0, sizeof(ldr_xfer_query));
+ ldr_xfer_query.hdr.command = LOADER_CMD_XFER_QUERY;
+ ldr_xfer_query.image_size = fw->size;
+ rv = loader_cl_send(client_data,
+ (u8 *)&ldr_xfer_query,
+ sizeof(ldr_xfer_query),
+ (u8 *)&ldr_xfer_query_resp,
+ sizeof(ldr_xfer_query_resp));
+ if (rv < 0) {
+ client_data->flag_retry = true;
+ return rv;
+ }
+
+ /* On success, the return value is the received buffer size */
+ if (rv != sizeof(struct loader_xfer_query_response)) {
+ dev_err(cl_data_to_dev(client_data),
+ "data size %d is not equal to size of loader_xfer_query_response %zu\n",
+ rv, sizeof(struct loader_xfer_query_response));
+ client_data->flag_retry = true;
+ return -EMSGSIZE;
+ }
+
+ /* Save fw_info for use outside this function */
+ *fw_info = ldr_xfer_query_resp.fw_info;
+
+ /* Loader firmware properties */
+ dev_dbg(cl_data_to_dev(client_data),
+ "ish_fw_version: major=%d minor=%d hotfix=%d build=%d protocol_version=0x%x loader_version=%d\n",
+ fw_info->ish_fw_version.major,
+ fw_info->ish_fw_version.minor,
+ fw_info->ish_fw_version.hotfix,
+ fw_info->ish_fw_version.build,
+ fw_info->protocol_version,
+ fw_info->ldr_version.value);
+
+ dev_dbg(cl_data_to_dev(client_data),
+ "loader_capability: max_fw_image_size=0x%x xfer_mode=%d max_dma_buf_size=0x%x dma_buf_size_limit=0x%x\n",
+ fw_info->ldr_capability.max_fw_image_size,
+ fw_info->ldr_capability.xfer_mode,
+ fw_info->ldr_capability.max_dma_buf_size,
+ dma_buf_size_limit);
+
+ /* Sanity checks */
+ if (fw_info->ldr_capability.max_fw_image_size < fw->size) {
+ dev_err(cl_data_to_dev(client_data),
+ "ISH firmware size %zu is greater than Shim firmware loader max supported %d\n",
+ fw->size,
+ fw_info->ldr_capability.max_fw_image_size);
+ return -ENOSPC;
+ }
+
+ /* For DMA the buffer size should be multiple of cacheline size */
+ if ((fw_info->ldr_capability.xfer_mode & LOADER_XFER_MODE_DIRECT_DMA) &&
+ (fw_info->ldr_capability.max_dma_buf_size % L1_CACHE_BYTES)) {
+ dev_err(cl_data_to_dev(client_data),
+ "Shim firmware loader buffer size %d should be multipe of cacheline\n",
+ fw_info->ldr_capability.max_dma_buf_size);
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+/**
+ * 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) {
+ client_data->flag_retry = true;
+ return -ENOMEM;
+ }
+
+ ldr_xfer_ipc_frag->fragment.hdr.command = LOADER_CMD_XFER_FRAGMENT;
+ ldr_xfer_ipc_frag->fragment.xfer_mode = LOADER_XFER_MODE_ISHTP;
+
+ /* Break the firmware image into fragments and send as ISH-TP payload */
+ fragment_offset = 0;
+ while (fragment_offset < fw->size) {
+ if (fragment_offset + payload_max_size < fw->size) {
+ fragment_size = payload_max_size;
+ ldr_xfer_ipc_frag->fragment.is_last = 0;
+ } else {
+ fragment_size = fw->size - fragment_offset;
+ ldr_xfer_ipc_frag->fragment.is_last = 1;
+ }
+
+ ldr_xfer_ipc_frag->fragment.offset = fragment_offset;
+ ldr_xfer_ipc_frag->fragment.size = fragment_size;
+ memcpy(ldr_xfer_ipc_frag->data,
+ &fw->data[fragment_offset],
+ fragment_size);
+
+ dev_dbg(cl_data_to_dev(client_data),
+ "xfer_mode=ipc offset=0x%08x size=0x%08x is_last=%d\n",
+ ldr_xfer_ipc_frag->fragment.offset,
+ ldr_xfer_ipc_frag->fragment.size,
+ ldr_xfer_ipc_frag->fragment.is_last);
+
+ rv = loader_cl_send(client_data,
+ (u8 *)ldr_xfer_ipc_frag,
+ IPC_FRAGMENT_DATA_PREAMBLE + fragment_size,
+ (u8 *)&ldr_xfer_ipc_ack,
+ sizeof(ldr_xfer_ipc_ack));
+ if (rv < 0) {
+ client_data->flag_retry = true;
+ goto end_err_resp_buf_release;
+ }
+
+ fragment_offset += fragment_size;
+ }
+
+ kfree(ldr_xfer_ipc_frag);
+ return 0;
+
+end_err_resp_buf_release:
+ /* Free ISH buffer if not done already, in error case */
+ kfree(ldr_xfer_ipc_frag);
+ return rv;
+}
+
+/**
+ * ish_fw_xfer_direct_dma() - Loads ISH firmware using direct dma
+ * @client_data: Client data instance
+ * @fw: Poiner to firmware data struct in host memory
+ *
+ * Host firmware load is a unique case where we need to download
+ * a large firmware image (200+ Kb). This function implements
+ * direct DMA transfer in kernel and ISH firmware. This allows
+ * us to overcome the ISH-TP 4 Kb limit, and allows us to DMA
+ * directly to ISH UMA at location of choice.
+ * Function depends on corresponding support in ISH firmware.
+ *
+ * Return: 0 for success, negative error code for failure.
+ */
+static int ish_fw_xfer_direct_dma(struct ishtp_cl_data *client_data,
+ const struct firmware *fw,
+ struct shim_fw_info fw_info)
+{
+ int rv;
+ void *dma_buf;
+ dma_addr_t dma_buf_phy;
+ u32 fragment_offset, fragment_size, payload_max_size;
+ struct loader_msg_hdr ldr_xfer_dma_frag_ack;
+ struct loader_xfer_dma_fragment ldr_xfer_dma_frag;
+ struct device *devc = ishtp_get_pci_device(client_data->cl_device);
+ u32 shim_fw_buf_size =
+ fw_info.ldr_capability.max_dma_buf_size;
+
+ /*
+ * 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) {
+ 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;
+
+ /* Send the firmware image in chucks of payload_max_size */
+ fragment_offset = 0;
+ while (fragment_offset < fw->size) {
+ if (fragment_offset + payload_max_size < fw->size) {
+ fragment_size = payload_max_size;
+ ldr_xfer_dma_frag.fragment.is_last = 0;
+ } else {
+ fragment_size = fw->size - fragment_offset;
+ ldr_xfer_dma_frag.fragment.is_last = 1;
+ }
+
+ ldr_xfer_dma_frag.fragment.offset = fragment_offset;
+ 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);
+
+ /*
+ * Flush cache here because the dma_sync_single_for_device()
+ * does not do for x86.
+ */
+ clflush_cache_range(dma_buf, payload_max_size);
+
+ dev_dbg(cl_data_to_dev(client_data),
+ "xfer_mode=dma offset=0x%08x size=0x%x is_last=%d ddr_phys_addr=0x%016llx\n",
+ ldr_xfer_dma_frag.fragment.offset,
+ ldr_xfer_dma_frag.fragment.size,
+ ldr_xfer_dma_frag.fragment.is_last,
+ ldr_xfer_dma_frag.ddr_phys_addr);
+
+ rv = loader_cl_send(client_data,
+ (u8 *)&ldr_xfer_dma_frag,
+ sizeof(ldr_xfer_dma_frag),
+ (u8 *)&ldr_xfer_dma_frag_ack,
+ sizeof(ldr_xfer_dma_frag_ack));
+ if (rv < 0) {
+ client_data->flag_retry = true;
+ goto end_err_resp_buf_release;
+ }
+
+ 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);
+ return rv;
+}
+
+/**
+ * ish_fw_start() Start executing ISH main firmware
+ * @client_data: client data instance
+ *
+ * This function sends message to Shim firmware loader to start
+ * the execution of ISH main firmware.
+ *
+ * Return: 0 for success, negative error code for failure.
+ */
+static int ish_fw_start(struct ishtp_cl_data *client_data)
+{
+ int rv;
+ struct loader_start ldr_start;
+ struct loader_msg_hdr ldr_start_ack;
+
+ memset(&ldr_start, 0, sizeof(ldr_start));
+ ldr_start.hdr.command = LOADER_CMD_START;
+ rv = loader_cl_send(client_data,
+ (u8 *)&ldr_start,
+ sizeof(ldr_start),
+ (u8 *)&ldr_start_ack,
+ sizeof(ldr_start_ack));
+
+ return rv;
+}
+
+/**
+ * load_fw_from_host() Loads ISH firmware from host
+ * @client_data: Client data instance
+ *
+ * This function loads the ISH firmware to ISH SRAM and starts execution
+ *
+ * Return: 0 for success, negative error code for failure.
+ */
+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) {
+ rv = -ENOMEM;
+ goto end_error;
+ }
+
+ /* Get filename of the ISH firmware to be loaded */
+ rv = get_firmware_variant(client_data, filename);
+ if (rv < 0)
+ goto end_err_filename_buf_release;
+
+ rv = request_firmware(&fw, filename, cl_data_to_dev(client_data));
+ if (rv < 0)
+ goto end_err_filename_buf_release;
+
+ /* Step 1: Query Shim firmware loader properties */
+
+ rv = ish_query_loader_prop(client_data, fw, &fw_info);
+ if (rv < 0)
+ goto end_err_fw_release;
+
+ /* Step 2: Send the main firmware image to be loaded, to ISH SRAM */
+
+ xfer_mode = fw_info.ldr_capability.xfer_mode;
+ if (xfer_mode & LOADER_XFER_MODE_DIRECT_DMA) {
+ rv = ish_fw_xfer_direct_dma(client_data, fw, fw_info);
+ } else if (xfer_mode & LOADER_XFER_MODE_ISHTP) {
+ rv = ish_fw_xfer_ishtp(client_data, fw);
+ } else {
+ dev_err(cl_data_to_dev(client_data),
+ "No transfer mode selected in firmware\n");
+ rv = -EINVAL;
+ }
+ if (rv < 0)
+ goto end_err_fw_release;
+
+ /* Step 3: Start ISH main firmware exeuction */
+
+ rv = ish_fw_start(client_data);
+ if (rv < 0)
+ goto end_err_fw_release;
+
+ release_firmware(fw);
+ kfree(filename);
+ dev_info(cl_data_to_dev(client_data), "ISH firmware %s loaded\n",
+ filename);
+ return 0;
+
+end_err_fw_release:
+ release_firmware(fw);
+end_err_filename_buf_release:
+ kfree(filename);
+end_error:
+ /* Keep a count of retries, and give up after 3 attempts */
+ if (client_data->flag_retry && client_data->retry_count++ < 3) {
+ dev_warn(cl_data_to_dev(client_data),
+ "ISH host firmware load failed %d. Reset ISH & try again..\n",
+ rv);
+ ish_hw_reset(ishtp_get_ishtp_device(loader_ishtp_cl));
+ } else {
+ dev_err(cl_data_to_dev(client_data),
+ "ISH host firmware load failed %d\n", rv);
+ }
+ return rv;
+}
+
+static void load_fw_from_host_handler(struct work_struct *work)
+{
+ struct ishtp_cl_data *client_data;
+
+ client_data = container_of(work, struct ishtp_cl_data,
+ work_fw_load);
+ load_fw_from_host(client_data);
+}
+
+/**
+ * loader_init() - Init function for ISH-TP client
+ * @loader_ishtp_cl: ISH-TP client instance
+ * @reset: true if called for init after reset
+ *
+ * Return: 0 for success, negative error code for failure
+ */
+static int loader_init(struct ishtp_cl *loader_ishtp_cl, int reset)
+{
+ int rv;
+ struct ishtp_fw_client *fw_client;
+ struct ishtp_cl_data *client_data =
+ ishtp_get_client_data(loader_ishtp_cl);
+
+ dev_dbg(cl_data_to_dev(client_data), "reset flag: %d\n", reset);
+
+ rv = ishtp_cl_link(loader_ishtp_cl);
+ if (rv < 0) {
+ dev_err(cl_data_to_dev(client_data), "ishtp_cl_link failed\n");
+ return rv;
+ }
+
+ /* Connect to firmware client */
+ ishtp_set_tx_ring_size(loader_ishtp_cl, LOADER_CL_TX_RING_SIZE);
+ ishtp_set_rx_ring_size(loader_ishtp_cl, LOADER_CL_RX_RING_SIZE);
+
+ fw_client =
+ ishtp_fw_cl_get_client(ishtp_get_ishtp_device(loader_ishtp_cl),
+ &loader_ishtp_guid);
+ if (!fw_client) {
+ dev_err(cl_data_to_dev(client_data),
+ "ISH client uuid not found\n");
+ rv = -ENOENT;
+ goto err_cl_unlink;
+ }
+
+ ishtp_cl_set_fw_client_id(loader_ishtp_cl,
+ ishtp_get_fw_client_id(fw_client));
+ ishtp_set_connection_state(loader_ishtp_cl, ISHTP_CL_CONNECTING);
+
+ rv = ishtp_cl_connect(loader_ishtp_cl);
+ if (rv < 0) {
+ dev_err(cl_data_to_dev(client_data), "Client connect fail\n");
+ goto err_cl_unlink;
+ }
+
+ dev_dbg(cl_data_to_dev(client_data), "Client connected\n");
+
+ ishtp_register_event_cb(client_data->cl_device, loader_cl_event_cb);
+
+ return 0;
+
+err_cl_unlink:
+ ishtp_cl_unlink(loader_ishtp_cl);
+ return rv;
+}
+
+static void loader_deinit(struct ishtp_cl *loader_ishtp_cl)
+{
+ ishtp_set_connection_state(loader_ishtp_cl, ISHTP_CL_DISCONNECTING);
+ ishtp_cl_disconnect(loader_ishtp_cl);
+ ishtp_cl_unlink(loader_ishtp_cl);
+ ishtp_cl_flush_queues(loader_ishtp_cl);
+
+ /* Disband and free all Tx and Rx client-level rings */
+ ishtp_cl_free(loader_ishtp_cl);
+}
+
+static void reset_handler(struct work_struct *work)
+{
+ int rv;
+ struct ishtp_cl_data *client_data;
+ struct ishtp_cl *loader_ishtp_cl;
+ struct ishtp_cl_device *cl_device;
+
+ client_data = container_of(work, struct ishtp_cl_data,
+ work_ishtp_reset);
+
+ loader_ishtp_cl = client_data->loader_ishtp_cl;
+ cl_device = client_data->cl_device;
+
+ /* Unlink, flush queues & start again */
+ ishtp_cl_unlink(loader_ishtp_cl);
+ ishtp_cl_flush_queues(loader_ishtp_cl);
+ ishtp_cl_free(loader_ishtp_cl);
+
+ loader_ishtp_cl = ishtp_cl_allocate(cl_device);
+ if (!loader_ishtp_cl)
+ return;
+
+ 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;
+
+ rv = loader_init(loader_ishtp_cl, 1);
+ if (rv < 0) {
+ dev_err(ishtp_device(cl_device), "Reset Failed\n");
+ return;
+ }
+
+ /* ISH firmware loading from host */
+ load_fw_from_host(client_data);
+}
+
+/**
+ * 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)
+ return -ENOMEM;
+
+ loader_ishtp_cl = ishtp_cl_allocate(cl_device);
+ if (!loader_ishtp_cl)
+ 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(&client_data->response.wait_queue);
+
+ INIT_WORK(&client_data->work_ishtp_reset,
+ reset_handler);
+ INIT_WORK(&client_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);
+
+ /* ISH firmware loading from host */
+ schedule_work(&client_data->work_fw_load);
+
+ return 0;
+}
+
+/**
+ * loader_ishtp_cl_remove() - ISH-TP client driver remove
+ * @cl_device: ISH-TP client device instance
+ *
+ * This function gets called on device remove on ISH-TP bus
+ *
+ * Return: 0
+ */
+static int loader_ishtp_cl_remove(struct ishtp_cl_device *cl_device)
+{
+ struct ishtp_cl_data *client_data;
+ struct ishtp_cl *loader_ishtp_cl = ishtp_get_drvdata(cl_device);
+
+ client_data = ishtp_get_client_data(loader_ishtp_cl);
+
+ /*
+ * The sequence of the following two cancel_work_sync() is
+ * important. The work_fw_load can in turn schedue
+ * work_ishtp_reset, so first cancel work_fw_load then
+ * cancel work_ishtp_reset.
+ */
+ cancel_work_sync(&client_data->work_fw_load);
+ cancel_work_sync(&client_data->work_ishtp_reset);
+ loader_deinit(loader_ishtp_cl);
+ ishtp_put_device(cl_device);
+
+ return 0;
+}
+
+/**
+ * loader_ishtp_cl_reset() - ISH-TP client driver reset
+ * @cl_device: ISH-TP client device instance
+ *
+ * This function gets called on device reset on ISH-TP bus
+ *
+ * Return: 0
+ */
+static int loader_ishtp_cl_reset(struct ishtp_cl_device *cl_device)
+{
+ struct ishtp_cl_data *client_data;
+ struct ishtp_cl *loader_ishtp_cl = ishtp_get_drvdata(cl_device);
+
+ client_data = ishtp_get_client_data(loader_ishtp_cl);
+
+ schedule_work(&client_data->work_ishtp_reset);
+
+ return 0;
+}
+
+static struct ishtp_cl_driver loader_ishtp_cl_driver = {
+ .name = "ish-loader",
+ .guid = &loader_ishtp_guid,
+ .probe = loader_ishtp_cl_probe,
+ .remove = loader_ishtp_cl_remove,
+ .reset = loader_ishtp_cl_reset,
+};
+
+static int __init ish_loader_init(void)
+{
+ return ishtp_cl_driver_register(&loader_ishtp_cl_driver, THIS_MODULE);
+}
+
+static void __exit ish_loader_exit(void)
+{
+ ishtp_cl_driver_unregister(&loader_ishtp_cl_driver);
+}
+
+late_initcall(ish_loader_init);
+module_exit(ish_loader_exit);
+
+module_param(dma_buf_size_limit, int, 0644);
+MODULE_PARM_DESC(dma_buf_size_limit, "Limit the DMA buf size to this value in bytes");
+
+MODULE_DESCRIPTION("ISH ISH-TP Host firmware Loader Client Driver");
+MODULE_AUTHOR("Rushikesh S Kadam <rushikesh.s.kadam@intel.com>");
+
+MODULE_LICENSE("GPL v2");
+MODULE_ALIAS("ishtp:*");
--
1.9.1
^ permalink raw reply related
* Re: [PATCH] HID: intel-ish-hid: ISH firmware loader client driver
From: Rushikesh S Kadam @ 2019-03-28 20:19 UTC (permalink / raw)
To: Nick Crews
Cc: benjamin.tissoires, jikos, jettrink, gwendal, linux-kernel,
linux-input, Srinivas Pandruvada
In-Reply-To: <CAHX4x87iQgjX8wHOYNCD7nYQJZKoirgCimPax11bMX_DGMEudw@mail.gmail.com>
Hi Nick
Thanks once again for your time.
I've addressed majority of the comments below.
Will send a v2 shortly. Please see my replies
inline below.
On Tue, Mar 26, 2019 at 06:39:14PM -0600, Nick Crews wrote:
> Hi Rushikesh, I know I've been reviewing this on Chromium, but I have
> some more larges-scale design thoughts.
>
> > > diff --git a/drivers/hid/intel-ish-hid/ishtp-fw-loader.c
> > > b/drivers/hid/intel-ish-hid/ishtp-fw-loader.c
> > > +/**
> > > + * report_bad_packets() Report bad packets
> > > + * @loader_ishtp_cl: Client instance to get stats
> > > + * @recv_buf: Raw received host interface message
> > > + *
> > > + * Dumps error in case bad packet is received
> > > + */
> > > +static void report_bad_packet(struct ishtp_cl *loader_ishtp_cl,
> > > + void *recv_buf)
> > > +{
> > > + struct loader_msg_hdr *hdr = recv_buf;
> > > + struct ishtp_cl_data *client_data =
> > > + ishtp_get_client_data(loader_ishtp_cl);
> > > +
> > > + client_data->bad_recv_cnt++;
> > > + dev_err(cl_data_to_dev(client_data),
> > > + "BAD packet: command=%02lx is_response=%u status=%02x
> > > total_bad=%u\n",
> > > + hdr->command & CMD_MASK,
> > > + hdr->command & IS_RESPONSE ? 1 : 0,
> > > + hdr->status,
> > > + client_data->bad_recv_cnt);
> > > +}
>
> I would remove this function. Whenever you call it, you already have
> use dev_err() to print the reason for the error. Consider removing
> bad_rcv_count too unless you do something with it other than debugging.
>
> At the very least, you call this function when the ISH doesn't return enough
> data, but in here you try to access the fields in hdr. This could be accessing
> irrelevant/illegal data.
>
> Also a nit: The docstring function name has a naughty trailing s.
I think I'll take your inputs to drop this
function.
>
> > > +
> > > +/**
> > > + * loader_ish_hw_reset() - Reset ISH HW in bad state
> > > + * @loader_ishtp_cl Client instance to reset
> > > + *
> > > + * This function resets ISH hardware, which shall reload
> > > + * the Shim firmware loader, initiate ISH-TP interface reset,
> > > + * re-attach kernel loader driver, and repeat Host
> > > + * firmware load.
> > > + */
> > > +static inline void loader_ish_hw_reset(struct ishtp_cl
> > > *loader_ishtp_cl)
> > > +{
> > > + struct ishtp_cl_data *client_data =
> > > + ishtp_get_client_data(loader_ishtp_cl);
> > > +
> > > + dev_warn(cl_data_to_dev(client_data), "Reset the ISH
> > > subsystem\n");
> > > + ish_hw_reset(ishtp_get_ishtp_device(loader_ishtp_cl));
> > > +}
>
> Delete this as a function. Before you actually called it in multiple places,
> but now i's only called in one place, so just inline it there.
Will drop this function as well.
>
> > > +
> > > +/**
> > > + * loader_cl_send() Send message from host to firmware
> > > + * @client_data: Client data instance
> > > + * @msg Message buffer to send
> > > + * @msg_size Size of message
> > > + *
> > > + * Return: Received buffer size on success, negative error code on
> > > failure.
> > > + */
> > > +static int loader_cl_send(struct ishtp_cl_data *client_data,
> > > + u8 *msg, size_t msg_size)
> > > +{
> > > + int rv;
> > > + size_t data_len;
> > > + struct loader_msg_hdr *in_hdr;
> > > + struct loader_msg_hdr *out_hdr = (struct loader_msg_hdr *)msg;
> > > + struct ishtp_cl *loader_ishtp_cl = client_data-
> > > >loader_ishtp_cl;
> > > +
> > > + dev_dbg(cl_data_to_dev(client_data),
> > > + "%s: command=%02lx is_response=%u status=%02x\n",
> > > + __func__,
> > > + out_hdr->command & CMD_MASK,
> > > + out_hdr->command & IS_RESPONSE ? 1 : 0,
> > > + out_hdr->status);
> > > +
> > > + client_data->flag_response = false;
> > > + rv = ishtp_cl_send(loader_ishtp_cl, msg, msg_size);
> > > + if (rv < 0) {
> > > + dev_err(cl_data_to_dev(client_data),
> > > + "ishtp_cl_send error %d\n", rv);
> > > + return rv;
> > > + }
> > > +
> > > + wait_event_interruptible_timeout(client_data->cmd_resp_wait,
> > > + client_data->flag_response,
> > > + ISHTP_SEND_TIMEOUT);
> > > + if (!client_data->flag_response) {
> > > + dev_err(cl_data_to_dev(client_data),
> > > + "Timed out for response to command=%02lx",
> > > + out_hdr->command & CMD_MASK);
> > > + return -ETIMEDOUT;
> > > + }
> > > +
> > > + /* All response messages will contain a header */
> > > + data_len = client_data->response_size;
> > > + in_hdr = (struct loader_msg_hdr *)client_data->response_data;
>
> If process_recv() fails then client_data->response_data could be NULL.
> This brings up the question of what to do if process_recv() fails. I would think
> that you would want it to set something like client_data->response_error
> and then you could check for that in here and return it. For instance
> right now if the ISH
> doesn't return sizeof(struct loader_msg_hdr) bytes then it would be nice to get
> -EMSGSIZE returned from here.
If the process_recv() fails, the flag_response
stays false. We check for the flag in calling
function and exit, so won't be accessing null
data.
But I do like your idea to add a resposne_error
field to pass the error to the calling function.
Will do so.
>
> > > +
> > > + /* Sanity checks */
> > > + if (!(in_hdr->command & IS_RESPONSE)) {
> > > + dev_err(cl_data_to_dev(client_data),
> > > + "Invalid response to command\n");
> > > + return -EIO;
> > > + }
> > > +
> > > + if (in_hdr->status) {
> > > + dev_err(cl_data_to_dev(client_data),
> > > + "Loader returned status %d\n",
> > > + in_hdr->status);
> > > + return -EIO;
> > > + }
> > > +
> > > + return data_len;
> > > +}
>
> So I think how you've changed this to handle where the data is stored is good,
> but it could be better. I don't like how the users of loader_cl_send() need to
> remember to kfree(client_data->response data), and that they just implicitly
> assume that client_data->response data holds the result. Instead, make the
> callers of loader_cl_send() allocate a buffer to hold the result, and then the
> allocating and freeing happens in the same function. I think this is a much more
> understandable form of memory management.
Agreed
>
> How about this function turns into:
> /**
> * loader_cl_send() Send message from host to firmware
> * @client_data: Client data instance
> * @in_data: Message buffer to send
> * @in_size: Size of sent data
> * @out_data: Buffer to fill with received data.
> * @out_size: Max number of bytes to place in out_data.
> *
> * Return: Number of bytes placed into out_data, negative error code on failure.
> */
Sounds good. One comment - should the names
out_msg & in_msg be interchanged? As in, the
message from AP to firmware be out_msg, and
firwmare to AP should be in_msg?
> static int loader_cl_send(struct ishtp_cl_data *client_data,
> u8 *in_data, size_t in_size,
> u8 *out_data, size_t out_size)
>
> {
> client_data->response_data = out_data;
> client_data->response_size_max = out_size;
>
> Send the command.
> Tweak process_recv() so where it does the memcpy() into
> client_data->response_data,
> add the additional check to make sure it doesn't copy more than
> client_data->response_size_max bytes.
> Wait for the completion flag.
> Continue with the rest.
> }
>
> With these suggestions there are now six pieces of info getting
> transmitted between
> process_recv() and loader_cl_send() via client data:
> client_data->cmd_resp_wait
> client_data->flag_response
> client_data->response_error
> client_data->response_size
> client_data->response_size_max
> client_data->response_data
> Consider turning these into:
> client_data->response_info->wait_queue
> client_data->response_info->data_available // or some better name?
> client_data->response_info->error
> client_data->response_info->size
> client_data->response_info->size_max
> client_data->response_info->data
> for some encapsulation?
Will do.
>
> I'm thinking about this more, and basically it seems like we're
> writing a library function to
> send a command to the ISH and receive a response. All the clients who
> use loader_cl_send()
> shouldn't know about the client_data->response_info stuff at all. It
> almost seems like this
> entire functionality should be part of the ISH core? It's really
> limiting that ishtp_cl_send() only
> allows sending and no receiving! It should?!
>
The way I see it is that the loader_cl_send() is
an application specific implementation. We make
assumptions here about the header (command,
is_response, status fields), and that all command
& response are serialized. Also this works well
when the response buffer is small, and we don't
mind copying content vs. passing pointer.
On the other hand, the ISH core provides a basic
but generic interface for ishtp_cl_send() and for
managing ring buffers.
If we could "standardize" loader_cl_send() and use
in more applications (such as upcoming driver for
cros_ec over ishtp), we may have a case to add as
a core function. I'll keep it in this driver for
the next revision (coming shortly). I'm open to
further discussion.
> > > +
> > > +/**
> > > + * process_recv() - Receive and parse incoming packet
> > > + * @loader_ishtp_cl: Client instance to get stats
> > > + * @rb_in_proc: ISH received message buffer
> > > + *
> > > + * Parse the incoming packet. If it is a response packet then it
> > > will
> > > + * update flag_response and wake up the caller waiting to for the
> > > response.
> > > + */
> > > +static void process_recv(struct ishtp_cl *loader_ishtp_cl,
> > > + struct ishtp_cl_rb *rb_in_proc)
> > > +{
> > > + size_t data_len = rb_in_proc->buf_idx;
> > > + struct loader_msg_hdr *hdr =
> > > + (struct loader_msg_hdr *)rb_in_proc->buffer.data;
> > > + struct ishtp_cl_data *client_data =
> > > + ishtp_get_client_data(loader_ishtp_cl);
> > > +
> > > + /*
> > > + * All firmware messages have a header. Check buffer size
> > > + * before accessing elements inside.
> > > + */
> > > + if (data_len < sizeof(struct loader_msg_hdr)) {
> > > + dev_err(cl_data_to_dev(client_data),
> > > + "data size %zu is less than header %zu\n",
> > > + data_len, sizeof(struct loader_msg_hdr));
> > > + report_bad_packet(client_data->loader_ishtp_cl, hdr);
> > > + goto end_error;
> > > + }
> > > +
> > > + dev_dbg(cl_data_to_dev(client_data),
> > > + "%s: command=%02lx is_response=%u status=%02x\n",
> > > + __func__,
> > > + hdr->command & CMD_MASK,
> > > + hdr->command & IS_RESPONSE ? 1 : 0,
> > > + hdr->status);
> > > +
> > > + switch (hdr->command & CMD_MASK) {
> > > + case LOADER_CMD_XFER_QUERY:
> > > + case LOADER_CMD_XFER_FRAGMENT:
> > > + case LOADER_CMD_START:
> > > + /* Sanity check */
> > > + if (client_data->response_data || client_data-
> > > >flag_response) {
>
> Following advice above, how about checking
> client_data->response_info->data_available instead?
> Or with advice above, corrupting old data might not be an issue,
> since the destination buffer changes? Also I wouldn't call this a buffer
> overrun below, it's a different problem.
I think I'll keep the check because even though
we may not be corrupting the buffer from the
previous call, it's still a bug that we got here
before processing the previous call.
> > > +/**
> > > + * loader_cl_event_cb() - bus driver callback for incoming message
> > > + * @device: Pointer to the the ishtp client device for
> > > which
> > > + * this message is targeted
> > > + *
> > > + * Remove the packet from the list and process the message by
> > > calling
> > > + * process_recv
> > > + */
> > > +static void loader_cl_event_cb(struct ishtp_cl_device *cl_device)
> > > +{
> > > + struct ishtp_cl_rb *rb_in_proc;
> > > + struct ishtp_cl_data *client_data;
> > > + struct ishtp_cl *loader_ishtp_cl =
> > > ishtp_get_drvdata(cl_device);
> > > +
> > > + client_data = ishtp_get_client_data(loader_ishtp_cl);
> > > +
> > > + while ((rb_in_proc = ishtp_cl_rx_get_rb(loader_ishtp_cl)) !=
> > > NULL) {
> > > + if (!rb_in_proc->buffer.data) {
> > > + dev_warn(cl_data_to_dev(client_data),
> > > + "rb_in_proc->buffer.data returned
> > > null");
>
> Maybe move this check into process_recv() and then you can set
> client_data->response_info->error for it?
Will do.
>
> > > + continue;
> > > + }
> > > +
> > > + /* Process the data packet from firmware */
> > > + process_recv(loader_ishtp_cl, rb_in_proc);
> > > + }
> > > +}
> > > +
> > > +/**
> > > + * ish_query_loader_prop() - Query ISH Shim firmware loader
> > > + * @client_data: Client data instance
> > > + * @fw: Poiner to fw data struct in host memory
> > > + *
> > > + * This function queries the ISH Shim firmware loader for
> > > capabilities.
> > > + *
> > > + * Return: 0 for success, negative error code for failure.
> > > + */
> > > +static int ish_query_loader_prop(struct ishtp_cl_data *client_data,
> > > + const struct firmware *fw,
> > > + struct shim_fw_info *fw_info)
> > > +{
> > > + int rv;
> > > + size_t data_len;
> > > + struct loader_msg_hdr *hdr;
> > > + struct loader_xfer_query ldr_xfer_query;
> > > + struct loader_xfer_query_response *ldr_xfer_query_resp;
> > > +
> > > + memset(&ldr_xfer_query, 0, sizeof(ldr_xfer_query));
> > > + ldr_xfer_query.hdr.command = LOADER_CMD_XFER_QUERY;
> > > + ldr_xfer_query.image_size = fw->size;
> > > + rv = loader_cl_send(client_data,
> > > + (u8 *)&ldr_xfer_query,
> > > + sizeof(ldr_xfer_query));
> > > + if (rv < 0) {
> > > + client_data->flag_retry = true;
> > > + goto end_error;
> > > + }
> > > +
> > > + /* Check buffer size before accessing the elements */
> > > + data_len = client_data->response_size;
>
> Use rv instead of client_data->response_size, we want to minimize weird
> unexplainable accesses of the fileds of client_data.
> Also consider not using the variable data_len, it doesn't do too much besides
> cause some indirection. With the change above it should be obvious
> what is going on.
I felt using data_len makes the code a bit easier
to read because it reminds the reader that the
returned value is the length of the received
buffer. But I'll make the change :)
> > > + release_firmware(fw);
> > > + kfree(filename);
> > > + dev_info(cl_data_to_dev(client_data), "ISH firmware %s
> > > loaded\n",
> > > + filename);
> > > + return 0;
> > > +
> > > +end_err_fw_release:
> > > + release_firmware(fw);
> > > +end_err_filename_buf_release:
> > > + kfree(filename);
> > > +end_error:
> > > + if (client_data->flag_retry) {
> > > + dev_warn(cl_data_to_dev(client_data),
> > > + "ISH host firmware load failed %d. Reset ISH &
> > > try again..\n",
> > > + rv);
> > > + loader_ish_hw_reset(client_data->loader_ishtp_cl);
>
> This could just keep failing infinitely, right? Do you want to add
> some retry counter,
> and after some limit then give up or something? What happens if the ISH firmware
> never succeeds in loading?
I'll add 3 attempts before we fail.
>
> > > + } else {
> > > + dev_err(cl_data_to_dev(client_data),
> > > + "ISH host firmware load failed %d\n", rv);
> > > + }
> > > + return rv;
> > > +}
>
> And there were many typos in comments that I saw, comb through them
> carefully again.
Let me scrub for them again.
>
> Cheers,
> Nick
Thanks
Rushikesh
--
^ permalink raw reply
* [PATCH] HID: force setting drvdata to NULL when removing the driver
From: Benjamin Tissoires @ 2019-03-28 15:51 UTC (permalink / raw)
To: Jiri Kosina, Bruno Prémont, Jonathan Cameron,
Srinivas Pandruvada, Hans de Goede, Jason Gerecke, Ping Cheng
Cc: linux-input, linux-kernel, Benjamin Tissoires
Or when the probe failed.
This is a common pattern in the HID drivers to reset the drvdata. Some
do it properly, some do it only in case of failure.
Anyway, it's never a good thing to have breadcrumbs, so force a clean
state when removing or when probe is failing.
Signed-off-by: Benjamin Tissoires <benjamin.tissoires@redhat.com>
---
drivers/hid/hid-core.c | 2 ++
drivers/hid/hid-cougar.c | 6 ++----
drivers/hid/hid-gfrm.c | 7 -------
drivers/hid/hid-lenovo.c | 2 --
drivers/hid/hid-logitech-dj.c | 2 --
drivers/hid/hid-logitech-hidpp.c | 8 +++-----
drivers/hid/hid-picolcd_core.c | 7 +------
drivers/hid/hid-sensor-hub.c | 1 -
drivers/hid/wacom_sys.c | 18 +++++-------------
9 files changed, 13 insertions(+), 40 deletions(-)
diff --git a/drivers/hid/hid-core.c b/drivers/hid/hid-core.c
index b5a8af8daa74..011e49da4d63 100644
--- a/drivers/hid/hid-core.c
+++ b/drivers/hid/hid-core.c
@@ -2203,6 +2203,7 @@ static int hid_device_probe(struct device *dev)
if (ret) {
hid_close_report(hdev);
hdev->driver = NULL;
+ hid_set_drvdata(hdev, NULL);
}
}
unlock:
@@ -2231,6 +2232,7 @@ static int hid_device_remove(struct device *dev)
else /* default remove */
hid_hw_stop(hdev);
hid_close_report(hdev);
+ hid_set_drvdata(hdev, NULL);
hdev->driver = NULL;
}
diff --git a/drivers/hid/hid-cougar.c b/drivers/hid/hid-cougar.c
index e0bb7b34f3a4..4ff3bc1d25e2 100644
--- a/drivers/hid/hid-cougar.c
+++ b/drivers/hid/hid-cougar.c
@@ -207,7 +207,7 @@ static int cougar_probe(struct hid_device *hdev,
error = hid_parse(hdev);
if (error) {
hid_err(hdev, "parse failed\n");
- goto fail;
+ return error;
}
if (hdev->collection->usage == COUGAR_VENDOR_USAGE) {
@@ -219,7 +219,7 @@ static int cougar_probe(struct hid_device *hdev,
error = hid_hw_start(hdev, connect_mask);
if (error) {
hid_err(hdev, "hw start failed\n");
- goto fail;
+ return error;
}
error = cougar_bind_shared_data(hdev, cougar);
@@ -249,8 +249,6 @@ static int cougar_probe(struct hid_device *hdev,
fail_stop_and_cleanup:
hid_hw_stop(hdev);
-fail:
- hid_set_drvdata(hdev, NULL);
return error;
}
diff --git a/drivers/hid/hid-gfrm.c b/drivers/hid/hid-gfrm.c
index cf477f8c8f4c..1a20997e7750 100644
--- a/drivers/hid/hid-gfrm.c
+++ b/drivers/hid/hid-gfrm.c
@@ -127,12 +127,6 @@ static int gfrm_probe(struct hid_device *hdev, const struct hid_device_id *id)
return ret;
}
-static void gfrm_remove(struct hid_device *hdev)
-{
- hid_hw_stop(hdev);
- hid_set_drvdata(hdev, NULL);
-}
-
static const struct hid_device_id gfrm_devices[] = {
{ HID_BLUETOOTH_DEVICE(0x58, 0x2000),
.driver_data = GFRM100 },
@@ -146,7 +140,6 @@ static struct hid_driver gfrm_driver = {
.name = "gfrm",
.id_table = gfrm_devices,
.probe = gfrm_probe,
- .remove = gfrm_remove,
.input_mapping = gfrm_input_mapping,
.raw_event = gfrm_raw_event,
.input_configured = gfrm_input_configured,
diff --git a/drivers/hid/hid-lenovo.c b/drivers/hid/hid-lenovo.c
index eacc76d2ab96..cde9d22bb3ec 100644
--- a/drivers/hid/hid-lenovo.c
+++ b/drivers/hid/hid-lenovo.c
@@ -869,8 +869,6 @@ static void lenovo_remove_tpkbd(struct hid_device *hdev)
led_classdev_unregister(&data_pointer->led_micmute);
led_classdev_unregister(&data_pointer->led_mute);
-
- hid_set_drvdata(hdev, NULL);
}
static void lenovo_remove_cptkbd(struct hid_device *hdev)
diff --git a/drivers/hid/hid-logitech-dj.c b/drivers/hid/hid-logitech-dj.c
index 826fa1e1c8d9..a75101293755 100644
--- a/drivers/hid/hid-logitech-dj.c
+++ b/drivers/hid/hid-logitech-dj.c
@@ -1094,7 +1094,6 @@ static int logi_dj_probe(struct hid_device *hdev,
hid_parse_fail:
kfifo_free(&djrcv_dev->notif_fifo);
kfree(djrcv_dev);
- hid_set_drvdata(hdev, NULL);
return retval;
}
@@ -1145,7 +1144,6 @@ static void logi_dj_remove(struct hid_device *hdev)
kfifo_free(&djrcv_dev->notif_fifo);
kfree(djrcv_dev);
- hid_set_drvdata(hdev, NULL);
}
static const struct hid_device_id logi_dj_receivers[] = {
diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
index 199cc256e9d9..b950a44157a2 100644
--- a/drivers/hid/hid-logitech-hidpp.c
+++ b/drivers/hid/hid-logitech-hidpp.c
@@ -3237,15 +3237,15 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
if (hidpp->quirks & HIDPP_QUIRK_CLASS_WTP) {
ret = wtp_allocate(hdev, id);
if (ret)
- goto allocate_fail;
+ return ret;
} else if (hidpp->quirks & HIDPP_QUIRK_CLASS_M560) {
ret = m560_allocate(hdev);
if (ret)
- goto allocate_fail;
+ return ret;
} else if (hidpp->quirks & HIDPP_QUIRK_CLASS_K400) {
ret = k400_allocate(hdev);
if (ret)
- goto allocate_fail;
+ return ret;
}
INIT_WORK(&hidpp->work, delayed_work_cb);
@@ -3343,8 +3343,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
sysfs_remove_group(&hdev->dev.kobj, &ps_attribute_group);
cancel_work_sync(&hidpp->work);
mutex_destroy(&hidpp->send_mutex);
-allocate_fail:
- hid_set_drvdata(hdev, NULL);
return ret;
}
diff --git a/drivers/hid/hid-picolcd_core.c b/drivers/hid/hid-picolcd_core.c
index c1b29a9eb41a..a5d30f267a7b 100644
--- a/drivers/hid/hid-picolcd_core.c
+++ b/drivers/hid/hid-picolcd_core.c
@@ -550,8 +550,7 @@ static int picolcd_probe(struct hid_device *hdev,
data = kzalloc(sizeof(struct picolcd_data), GFP_KERNEL);
if (data == NULL) {
hid_err(hdev, "can't allocate space for Minibox PicoLCD device data\n");
- error = -ENOMEM;
- goto err_no_cleanup;
+ return -ENOMEM;
}
spin_lock_init(&data->lock);
@@ -613,9 +612,6 @@ static int picolcd_probe(struct hid_device *hdev,
hid_hw_stop(hdev);
err_cleanup_data:
kfree(data);
-err_no_cleanup:
- hid_set_drvdata(hdev, NULL);
-
return error;
}
@@ -651,7 +647,6 @@ static void picolcd_remove(struct hid_device *hdev)
picolcd_exit_cir(data);
picolcd_exit_keys(data);
- hid_set_drvdata(hdev, NULL);
mutex_destroy(&data->mutex);
/* Finally, clean up the picolcd data itself */
kfree(data);
diff --git a/drivers/hid/hid-sensor-hub.c b/drivers/hid/hid-sensor-hub.c
index 4256fdc5cd6d..a3db5d8b382d 100644
--- a/drivers/hid/hid-sensor-hub.c
+++ b/drivers/hid/hid-sensor-hub.c
@@ -755,7 +755,6 @@ static void sensor_hub_remove(struct hid_device *hdev)
}
spin_unlock_irqrestore(&data->lock, flags);
mfd_remove_devices(&hdev->dev);
- hid_set_drvdata(hdev, NULL);
mutex_destroy(&data->mutex);
}
diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
index a8633b1437b2..c2fb614bff44 100644
--- a/drivers/hid/wacom_sys.c
+++ b/drivers/hid/wacom_sys.c
@@ -2716,14 +2716,12 @@ static int wacom_probe(struct hid_device *hdev,
wacom_wac->features = *((struct wacom_features *)id->driver_data);
features = &wacom_wac->features;
- if (features->check_for_hid_type && features->hid_type != hdev->type) {
- error = -ENODEV;
- goto fail;
- }
+ if (features->check_for_hid_type && features->hid_type != hdev->type)
+ return -ENODEV;
error = kfifo_alloc(&wacom_wac->pen_fifo, WACOM_PKGLEN_MAX, GFP_KERNEL);
if (error)
- goto fail;
+ return error;
wacom_wac->hid_data.inputmode = -1;
wacom_wac->mode_report = -1;
@@ -2741,12 +2739,12 @@ static int wacom_probe(struct hid_device *hdev,
error = hid_parse(hdev);
if (error) {
hid_err(hdev, "parse failed\n");
- goto fail;
+ return error;
}
error = wacom_parse_and_register(wacom, false);
if (error)
- goto fail;
+ return error;
if (hdev->bus == BUS_BLUETOOTH) {
error = device_create_file(&hdev->dev, &dev_attr_speed);
@@ -2757,10 +2755,6 @@ static int wacom_probe(struct hid_device *hdev,
}
return 0;
-
-fail:
- hid_set_drvdata(hdev, NULL);
- return error;
}
static void wacom_remove(struct hid_device *hdev)
@@ -2789,8 +2783,6 @@ static void wacom_remove(struct hid_device *hdev)
wacom_release_resources(wacom);
kfifo_free(&wacom_wac->pen_fifo);
-
- hid_set_drvdata(hdev, NULL);
}
#ifdef CONFIG_PM
--
2.19.2
^ permalink raw reply related
* Re: [PATCH v4] HID: core: move Usage Page concatenation to Main item
From: Benjamin Tissoires @ 2019-03-28 13:48 UTC (permalink / raw)
To: Junge, Terry
Cc: Nicolas Saenz Julienne, Jiri Kosina, oneukum@suse.de,
linux-input@vger.kernel.org, linux-kernel@vger.kernel.org
In-Reply-To: <BYAPR02MB5607E9396127F67A9A2BEC948A590@BYAPR02MB5607.namprd02.prod.outlook.com>
On Thu, Mar 28, 2019 at 1:27 AM Junge, Terry <terry.junge@poly.com> wrote:
>
> Hi Nicolas,
>
> V4 looks good to me.
Looks good to me too.
Terry, can I consider this as a formal Rev-by you?
Cheers,
Benjamin
>
> Thanks,
> Terry
>
> >-----Original Message-----
> >Subject: [PATCH v4] HID: core: move Usage Page concatenation to Main item
> >
^ permalink raw reply
* Re: [PATCH v3 3/4] driver core: add dev_print_hex_dump() logging function.
From: Steven Rostedt @ 2019-03-28 12:30 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Life is hard, and then you die, Dmitry Torokhov, Henrik Rydberg,
Andy Shevchenko, Sergey Senozhatsky, Rafael J. Wysocki,
Lukas Wunner, Federico Lorenzi, linux-input, linux-kernel
In-Reply-To: <20190328112952.GA2232@kroah.com>
On Thu, 28 Mar 2019 12:29:52 +0100
Greg Kroah-Hartman <gregkh@linuxfoundation.org> wrote:
> > 3. In at least two places we log different types of packets in the
> > same lines of code (protected by a "if (log_mask & PKT_TYPE)") -
> > dynamic-debug would only allow enabling or disabling logging of
> > all packets. This could be worked around by creating a separate
> > (but essentially duplicate) logging function for each packet type
> > and some lookup table to call the appropriate one. Not very
> > pretty IMO, though.
>
> True, but you can use tracepoints as well, that would probably be much
> easier when you are logging data streams. You can also use an ebpf
> program with the tracepoints to log just what you need/want to when you
> want to as well.
And tracepoints have filters where you don't even need ebpf to do such
filtering.
See Documentation/trace/events.rst Section 5: Event Filtering
-- Steve
^ permalink raw reply
* Re: Re: [PATCH v3 1/4] drm/bridge: sil_sii8620: depend on INPUT instead of selecting it.
From: Andrzej Hajda @ 2019-03-28 11:48 UTC (permalink / raw)
To: Life is hard, and then you die
Cc: Dmitry Torokhov, Henrik Rydberg, Andy Shevchenko,
Sergey Senozhatsky, Steven Rostedt, Greg Kroah-Hartman,
Rafael J. Wysocki, Lukas Wunner, Federico Lorenzi, linux-input,
linux-kernel, Inki Dae, dri-devel@lists.freedesktop.org
In-Reply-To: <20190328000743.GB24753@innovation.ch>
On 28.03.2019 01:07, Life is hard, and then you die wrote:
> Hi Andrzej,
>
> On Wed, Mar 27, 2019 at 03:13:37PM +0100, Andrzej Hajda wrote:
>> +cc: dri-devel
>>
>> On 27.03.2019 02:48, Ronald Tschalär wrote:
>>> commit d6abe6df706c66d803e6dd4fe98c1b6b7f125a56 (drm/bridge:
>>> sil_sii8620: do not have a dependency of RC_CORE) added a dependency on
>>> INPUT. However, this causes problems with other drivers, in particular
>>> an input driver that depends on MFD_INTEL_LPSS_PCI (to be added in a
>>> future commit):
>>>
>>> drivers/clk/Kconfig:9:error: recursive dependency detected!
>>> drivers/clk/Kconfig:9: symbol COMMON_CLK is selected by MFD_INTEL_LPSS
>>> drivers/mfd/Kconfig:566: symbol MFD_INTEL_LPSS is selected by MFD_INTEL_LPSS_PCI
>>> drivers/mfd/Kconfig:580: symbol MFD_INTEL_LPSS_PCI is implied by KEYBOARD_APPLESPI
>>> drivers/input/keyboard/Kconfig:73: symbol KEYBOARD_APPLESPI depends on INPUT
>>> drivers/input/Kconfig:8: symbol INPUT is selected by DRM_SIL_SII8620
>>> drivers/gpu/drm/bridge/Kconfig:83: symbol DRM_SIL_SII8620 depends on DRM_BRIDGE
>>> drivers/gpu/drm/bridge/Kconfig:1: symbol DRM_BRIDGE is selected by DRM_PL111
>>> drivers/gpu/drm/pl111/Kconfig:1: symbol DRM_PL111 depends on COMMON_CLK
>>>
>>> According to the docs, select should only be used for non-visible
>>> symbols. Furthermore almost all other references to INPUT throughout the
>>> kernel config are depends, not selects. Hence this change.
>>>
>>> CC: Inki Dae <inki.dae@samsung.com>
>>> CC: Andrzej Hajda <a.hajda@samsung.com>
>>> Signed-off-by: Ronald Tschalär <ronald@innovation.ch>
>> This is drm bridge driver, next time please cc it to dri-devel ML also.
> Ok. Though as noted in the cover letter, the patch here is meant as a
> placeholder till the real thing being discussed on dri-devel is
> finalized. I was trying to avoid cross-posting too much, hence the
> separate submission on dri-devel.
>
>> Anyway this is not the solution we have agreed to.
>>
>> Why have you abandoned the patch [1]? It needed just some minor
>> polishing, as I wrote in response to this mail.
>>
>> [1]:
>> https://lore.kernel.org/lkml/20190124082423.23139-1-ronald@innovation.ch/T/#mf620df0b1583096a214d8e2e690387078583472f
> It seems your mail client doesn't like me :-) I got neither of your
> responses. Sorry, I should've checked the archives when I didn't hear
> anything. In any case thank you for your review, and I will update
> that patch and send out a new version shortly.
I see where is the problem: Your mail client (mutt I suppose) sets
Mail-Followup-To header to all recipients (To and Cc) without the
sender. And my client (thunderbird) after pressing "Reply-All" checks
for Mail-Followup-To field, if present it uses only it to set
recipients, so in your case it does not response to the original author.
I do not know which mail client works incorrectly in this case, but for
sure it is not what we want :)
I do not know how to solve the issue in thunderbird, maybe mutt is more
configurable?
Regards
Andrzej
>
>
> Cheers,
>
> Ronald
>
>
>>> ---
>>> drivers/gpu/drm/bridge/Kconfig | 2 +-
>>> 1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/bridge/Kconfig b/drivers/gpu/drm/bridge/Kconfig
>>> index 2fee47b0d50b..eabedc83f25c 100644
>>> --- a/drivers/gpu/drm/bridge/Kconfig
>>> +++ b/drivers/gpu/drm/bridge/Kconfig
>>> @@ -83,9 +83,9 @@ config DRM_PARADE_PS8622
>>> config DRM_SIL_SII8620
>>> tristate "Silicon Image SII8620 HDMI/MHL bridge"
>>> depends on OF
>>> + depends on INPUT
>>> select DRM_KMS_HELPER
>>> imply EXTCON
>>> - select INPUT
>>> select RC_CORE
>>> help
>>> Silicon Image SII8620 HDMI/MHL bridge chip driver.
>>
>
^ permalink raw reply
* Re: [PATCH v3 3/4] driver core: add dev_print_hex_dump() logging function.
From: Greg Kroah-Hartman @ 2019-03-28 11:29 UTC (permalink / raw)
To: Life is hard, and then you die
Cc: Dmitry Torokhov, Henrik Rydberg, Andy Shevchenko,
Sergey Senozhatsky, Steven Rostedt, Rafael J. Wysocki,
Lukas Wunner, Federico Lorenzi, linux-input, linux-kernel
In-Reply-To: <20190328102755.GA26446@innovation.ch>
On Thu, Mar 28, 2019 at 03:27:55AM -0700, Life is hard, and then you die wrote:
>
> On Thu, Mar 28, 2019 at 06:29:17AM +0100, Greg Kroah-Hartman wrote:
> > On Wed, Mar 27, 2019 at 05:28:17PM -0700, Life is hard, and then you die wrote:
> > >
> > > On Wed, Mar 27, 2019 at 11:37:57AM +0900, Greg Kroah-Hartman wrote:
> > > > On Tue, Mar 26, 2019 at 06:48:06PM -0700, Ronald Tschalär wrote:
> > > > > This is the dev_xxx() analog to print_hex_dump(), using dev_printk()
> > > > > instead of straight printk() to match other dev_xxx() logging functions.
> > > > > ---
> > > > > drivers/base/core.c | 43 ++++++++++++++++++++++++++++++++++++++++++
> > > > > include/linux/device.h | 15 +++++++++++++++
> > > > > 2 files changed, 58 insertions(+)
> > > >
> > > > No signed-off-by?
> > >
> > > Aargh! Apologies, fixed for the future.
> > >
> > > > Anyway, no, please do not do this. Please do not dump large hex values
> > > > like this to the kernel log, it does not help anyone.
> > > >
> > > > You can do this while debugging, sure, but not for "real" kernel code.
> > >
> > > As used by this driver, it is definitely called for debugging only and
> > > must be explicitly enabled via a module param. But having the ability
> > > for folks to easily generate and print out debugging info has proven
> > > quite valuable.
> >
> > We have dynamic debugging, no need for module parameters at all. This
> > isn't the 1990's anymore :)
>
> I am aware of dynamic debugging, but there are several issues with it
> from the perspective of the logging I'm doing here (I mentioned these
> in response to an earlier review already):
>
> 1. Dynamic debugging can't be enabled at a function or line level on
> the kernel command line, so this means that to debug issues
> during boot you have to enable everything, which can be way too
> verbose
You can, and should enable it at a function or line level by writing to
the proper kernel file in debugfs.
You have read Documentation/admin-guide/dynamic-debug-howto.rst, right?
No need to do anything on the command line, that's so old-school :)
> 2. The expressions to enable the individual logging statements are
> quite brittle (in particular the line-based ones) since they
> (may) change any time the code is changed - having stable
> commands to give to users and put in documentation (e.g.
> "echo 0x200 > /sys/module/applespi/parameters/debug") is
> quite valuable.
>
> One way to work around this would be to put every single logging
> statement in a function of its own, thereby ensuring a stable
> name is associated with each one.
Again, read the documentation, this works today as-is.
> 3. In at least two places we log different types of packets in the
> same lines of code (protected by a "if (log_mask & PKT_TYPE)") -
> dynamic-debug would only allow enabling or disabling logging of
> all packets. This could be worked around by creating a separate
> (but essentially duplicate) logging function for each packet type
> and some lookup table to call the appropriate one. Not very
> pretty IMO, though.
True, but you can use tracepoints as well, that would probably be much
easier when you are logging data streams. You can also use an ebpf
program with the tracepoints to log just what you need/want to when you
want to as well.
> 4. In a couple places we log both the sent command and the received
> response, with the log-mask determining for which types of
> packets to do this. With a log mask there is one bit to set to
> get both logged; with dynamic debugging you'd have to enable the
> writing and receiving parts separately (not a huge deal, but yet
> one place where things are a bit more complicated than
> necessary).
>
> Except for the first, none of these are insurmountable, but together
> they don't make dynamic debugging very attractive for this sort of
> logging.
Look into it some more, we have all of this covered today, no need for
just a single driver to do something fancy on its own :)
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH v3 2/4] lib/hexdump.c: factor out generic hexdump formatting for reuse.
From: Life is hard, and then you die @ 2019-03-28 10:29 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Dmitry Torokhov, Henrik Rydberg, Sergey Senozhatsky,
Steven Rostedt, Greg Kroah-Hartman, Rafael J. Wysocki,
Lukas Wunner, Federico Lorenzi, linux-input,
Linux Kernel Mailing List
In-Reply-To: <20190328090350.GS9224@smile.fi.intel.com>
On Thu, Mar 28, 2019 at 11:03:50AM +0200, Andy Shevchenko wrote:
> On Wed, Mar 27, 2019 at 05:34:59PM -0700, Life is hard, and then you die wrote:
> >
> > On Wed, Mar 27, 2019 at 09:46:48AM +0200, Andy Shevchenko wrote:
> > > On Wed, Mar 27, 2019 at 3:49 AM Ronald Tschalär <ronald@innovation.ch> wrote:
> > > >
> > > > This introduces print_hex_dump_to_cb() which contains all the hexdump
> > > > formatting minus the actual printk() call, allowing an arbitrary print
> > > > function to be supplied instead. And print_hex_dump() is re-implemented
> > > > using print_hex_dump_to_cb().
> > > >
> > > > This allows other hex-dump logging functions to be provided which call
> > > > printk() differently or even log the hexdump somewhere entirely
> > > > different.
>
> > > In any case, don't do it like this. smaller non-recursive printf() is
> > > better than one big receursive call.
> > > When it looks like an optimization, it's actually a regression.
> >
> > Not sure where you see recursion here - are you referring to the
> > callback approach?
>
> %pV is a recursive printf().
Ah!
> > Since dev_printk() ends up calling printk with a
> > dictionary as well as additional formatting, vs print_hex_dump()'s
> > stright use of printk, this seemed like the best way accommodate
> > various possible ways of logging the messages. But as per below I
> > guess this is moot.
>
> I recommend to read this: https://lwn.net/Articles/780556/
Thanks, quite informative.
Cheers,
Ronald
^ permalink raw reply
* Re: [PATCH v3 3/4] driver core: add dev_print_hex_dump() logging function.
From: Life is hard, and then you die @ 2019-03-28 10:27 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Dmitry Torokhov, Henrik Rydberg, Andy Shevchenko,
Sergey Senozhatsky, Steven Rostedt, Rafael J. Wysocki,
Lukas Wunner, Federico Lorenzi, linux-input, linux-kernel
In-Reply-To: <20190328052917.GB18668@kroah.com>
On Thu, Mar 28, 2019 at 06:29:17AM +0100, Greg Kroah-Hartman wrote:
> On Wed, Mar 27, 2019 at 05:28:17PM -0700, Life is hard, and then you die wrote:
> >
> > On Wed, Mar 27, 2019 at 11:37:57AM +0900, Greg Kroah-Hartman wrote:
> > > On Tue, Mar 26, 2019 at 06:48:06PM -0700, Ronald Tschalär wrote:
> > > > This is the dev_xxx() analog to print_hex_dump(), using dev_printk()
> > > > instead of straight printk() to match other dev_xxx() logging functions.
> > > > ---
> > > > drivers/base/core.c | 43 ++++++++++++++++++++++++++++++++++++++++++
> > > > include/linux/device.h | 15 +++++++++++++++
> > > > 2 files changed, 58 insertions(+)
> > >
> > > No signed-off-by?
> >
> > Aargh! Apologies, fixed for the future.
> >
> > > Anyway, no, please do not do this. Please do not dump large hex values
> > > like this to the kernel log, it does not help anyone.
> > >
> > > You can do this while debugging, sure, but not for "real" kernel code.
> >
> > As used by this driver, it is definitely called for debugging only and
> > must be explicitly enabled via a module param. But having the ability
> > for folks to easily generate and print out debugging info has proven
> > quite valuable.
>
> We have dynamic debugging, no need for module parameters at all. This
> isn't the 1990's anymore :)
I am aware of dynamic debugging, but there are several issues with it
from the perspective of the logging I'm doing here (I mentioned these
in response to an earlier review already):
1. Dynamic debugging can't be enabled at a function or line level on
the kernel command line, so this means that to debug issues
during boot you have to enable everything, which can be way too
verbose
2. The expressions to enable the individual logging statements are
quite brittle (in particular the line-based ones) since they
(may) change any time the code is changed - having stable
commands to give to users and put in documentation (e.g.
"echo 0x200 > /sys/module/applespi/parameters/debug") is
quite valuable.
One way to work around this would be to put every single logging
statement in a function of its own, thereby ensuring a stable
name is associated with each one.
3. In at least two places we log different types of packets in the
same lines of code (protected by a "if (log_mask & PKT_TYPE)") -
dynamic-debug would only allow enabling or disabling logging of
all packets. This could be worked around by creating a separate
(but essentially duplicate) logging function for each packet type
and some lookup table to call the appropriate one. Not very
pretty IMO, though.
4. In a couple places we log both the sent command and the received
response, with the log-mask determining for which types of
packets to do this. With a log mask there is one bit to set to
get both logged; with dynamic debugging you'd have to enable the
writing and receiving parts separately (not a huge deal, but yet
one place where things are a bit more complicated than
necessary).
Except for the first, none of these are insurmountable, but together
they don't make dynamic debugging very attractive for this sort of
logging.
Cheers,
Ronald
^ permalink raw reply
* Re: [PATCH] Input: i8042 - signal wakeup from atkbd/psmouse
From: Rafael J. Wysocki @ 2019-03-28 10:26 UTC (permalink / raw)
To: Dmitry Torokhov
Cc: linux-input, Ravi Chandra Sadineni, Rafael J. Wysocki,
Shaunak Saha, linux-kernel
In-Reply-To: <20190327002800.GA187083@dtor-ws>
On Wednesday, March 27, 2019 1:28:00 AM CET Dmitry Torokhov wrote:
> Instead of signalling wakeup directly from i8042, let psmouse and atkbd
> drivers execute basic protocol handling and only then signal wakeup
> condition. This solves the issue where we increment wakeup counter
> simply because we are getting responses from keyboard/mouse to the
> commands we ourselves send to them as part of suspend transition.
>
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
LGTM:
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
> drivers/input/keyboard/atkbd.c | 2 ++
> drivers/input/mouse/psmouse-base.c | 2 ++
> drivers/input/serio/i8042.c | 3 ---
> 3 files changed, 4 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/input/keyboard/atkbd.c b/drivers/input/keyboard/atkbd.c
> index 850bb259c20e..3ad93e3e2f4c 100644
> --- a/drivers/input/keyboard/atkbd.c
> +++ b/drivers/input/keyboard/atkbd.c
> @@ -401,6 +401,8 @@ static irqreturn_t atkbd_interrupt(struct serio *serio, unsigned char data,
> if (ps2_handle_response(&atkbd->ps2dev, data))
> goto out;
>
> + pm_wakeup_event(&serio->dev, 0);
> +
> if (!atkbd->enabled)
> goto out;
>
> diff --git a/drivers/input/mouse/psmouse-base.c b/drivers/input/mouse/psmouse-base.c
> index d3ff1fc09af7..94f7ca5ad077 100644
> --- a/drivers/input/mouse/psmouse-base.c
> +++ b/drivers/input/mouse/psmouse-base.c
> @@ -373,6 +373,8 @@ static irqreturn_t psmouse_interrupt(struct serio *serio,
> if (ps2_handle_response(&psmouse->ps2dev, data))
> goto out;
>
> + pm_wakeup_event(&serio->dev, 0);
> +
> if (psmouse->state <= PSMOUSE_RESYNCING)
> goto out;
>
> diff --git a/drivers/input/serio/i8042.c b/drivers/input/serio/i8042.c
> index 95a78ccbd847..6462f1798fbb 100644
> --- a/drivers/input/serio/i8042.c
> +++ b/drivers/input/serio/i8042.c
> @@ -573,9 +573,6 @@ static irqreturn_t i8042_interrupt(int irq, void *dev_id)
> port = &i8042_ports[port_no];
> serio = port->exists ? port->serio : NULL;
>
> - if (irq && serio)
> - pm_wakeup_event(&serio->dev, 0);
> -
> filter_dbg(port->driver_bound, data, "<- i8042 (interrupt, %d, %d%s%s)\n",
> port_no, irq,
> dfl & SERIO_PARITY ? ", bad parity" : "",
>
^ permalink raw reply
* [PATCH RESEND -next] Input: uinput - Avoid Use-After-Free with udev lock
From: Mukesh Ojha @ 2019-03-28 10:25 UTC (permalink / raw)
To: linux-input, linux-kernel
Cc: Mukesh Ojha, Gaurav Kohli, Peter Hutterer, Martin Kepplinger,
Paul E. McKenney
uinput_destroy_device() gets called from two places. In one place,
uinput_ioctl_handler() it is protected under a lock udev->mutex
but same is not true for other place inside uinput_release().
This can result in a race where udev device gets freed while it
is in use.
[ 160.093398] Call trace:
[ 160.093417] kernfs_get+0x64/0x88
[ 160.093438] kernfs_new_node+0x94/0xc8
[ 160.093450] kernfs_create_dir_ns+0x44/0xfc
[ 160.093463] sysfs_create_dir_ns+0xa8/0x130
[ 160.093479] kobject_add_internal+0x278/0x650
[ 160.093491] kobject_add_varg+0xe0/0x130
[ 160.093502] kobject_add+0x15c/0x1d0
[ 160.093518] device_add+0x2bc/0xde0
[ 160.093533] input_register_device+0x5f4/0xa0c
[ 160.093547] uinput_ioctl_handler+0x1184/0x2198
[ 160.093560] uinput_ioctl+0x38/0x48
[ 160.093573] vfs_ioctl+0x7c/0xb4
[ 160.093585] do_vfs_ioctl+0x9ec/0x2350
[ 160.093597] SyS_ioctl+0x6c/0xa4
[ 160.093610] el0_svc_naked+0x34/0x38
[ 160.093621] ---[ end trace bccf0093cda2c538 ]---
[ 160.099041] =============================================================================
[ 160.107459] BUG kernfs_node_cache (Tainted: G S W O ): Object already free
[ 160.115235] -----------------------------------------------------------------------------
[ 160.115235]
[ 160.125151] Disabling lock debugging due to kernel taint
[ 160.130626] INFO: Allocated in __kernfs_new_node+0x8c/0x3c0 age=11 cpu=2 pid=7098
[ 160.138314] kmem_cache_alloc+0x358/0x388
[ 160.142445] __kernfs_new_node+0x8c/0x3c0
[ 160.146590] kernfs_new_node+0x80/0xc8
[ 160.150462] kernfs_create_dir_ns+0x44/0xfc
[ 160.154777] sysfs_create_dir_ns+0xa8/0x130
[ 160.158416] CPU5: update max cpu_capacity 1024
[ 160.159085] kobject_add_internal+0x278/0x650
[ 160.163567] kobject_add_varg+0xe0/0x130
[ 160.167606] kobject_add+0x15c/0x1d0
[ 160.168452] CPU5: update max cpu_capacity 780
[ 160.171287] get_device_parent+0x2d0/0x34c
[ 160.175510] device_add+0x240/0xde0
[ 160.178371] CPU6: update max cpu_capacity 916
[ 160.179108] input_register_device+0x5f4/0xa0c
[ 160.183686] uinput_ioctl_handler+0x1184/0x2198
[ 160.188346] uinput_ioctl+0x38/0x48
[ 160.191941] vfs_ioctl+0x7c/0xb4
[ 160.195261] do_vfs_ioctl+0x9ec/0x2350
[ 160.199111] SyS_ioctl+0x6c/0xa4
[ 160.202436] INFO: Freed in kernfs_put+0x2c8/0x434 age=14 cpu=0 pid=7096
[ 160.209230] kernfs_put+0x2c8/0x434
[ 160.212825] kobject_del+0x50/0xcc
[ 160.216332] cleanup_glue_dir+0x124/0x16c
[ 160.220456] device_del+0x55c/0x5c8
[ 160.224047] __input_unregister_device+0x274/0x2a8
[ 160.228974] input_unregister_device+0x90/0xd0
[ 160.233553] uinput_destroy_device+0x15c/0x1dc
[ 160.238131] uinput_release+0x44/0x5c
[ 160.241898] __fput+0x1f4/0x4e4
[ 160.245127] ____fput+0x20/0x2c
[ 160.248358] task_work_run+0x9c/0x174
[ 160.252127] do_notify_resume+0x104/0x6bc
[ 160.256253] work_pending+0x8/0x14
[ 160.259751] INFO: Slab 0xffffffbf0215ff00 objects=33 used=11 fp=0xffffffc0857ffd08 flags=0x8101
[ 160.268693] INFO: Object 0xffffffc0857ffd08 @offset=15624 fp=0xffffffc0857fefb0
[ 160.268693]
[ 160.277721] Redzone ffffffc0857ffd00: bb bb bb bb bb bb bb bb ........
[ 160.286656] Object ffffffc0857ffd08: 00 00 00 00 01 00 00 80 58 a2 37 45 c1 ff ff ff ........X.7E....
[ 160.296207] Object ffffffc0857ffd18: ae 21 10 0b 90 ff ff ff 20 fd 7f 85 c0 ff ff ff .!...... .......
[ 160.305780] Object ffffffc0857ffd28: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 160.315342] Object ffffffc0857ffd38: 00 00 00 00 00 00 00 00 7d a3 25 69 00 00 00 00 ........}.%i....
[ 160.324896] Object ffffffc0857ffd48: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 160.334446] Object ffffffc0857ffd58: 80 c0 28 47 c1 ff ff ff 00 00 00 00 00 00 00 00 ..(G............
[ 160.344000] Object ffffffc0857ffd68: 80 4a ce d1 c0 ff ff ff dc 32 01 00 01 00 00 00 .J.......2......
[ 160.353554] Object ffffffc0857ffd78: 11 00 ed 41 00 00 00 00 00 00 00 00 00 00 00 00 ...A............
[ 160.363099] Redzone ffffffc0857ffd88: bb bb bb bb bb bb bb bb ........
[ 160.372032] Padding ffffffc0857ffee0: 5a 5a 5a 5a 5a 5a 5a 5a ZZZZZZZZ
[ 160.378299] CPU6: update max cpu_capacity 780
[ 160.380971] CPU: 4 PID: 7098 Comm: syz-executor Tainted: G S B W O 4.14.98+ #1
So, avoid the race by taking a udev lock inside `uinput_release()`.
Change-Id: I3bbb07589b7b6e0e1b3bea572b5eb4f6b09774d6
Signed-off-by: Mukesh Ojha <mojha@codeaurora.org>
Cc: Gaurav Kohli <gkohli@codeaurora.org>
Cc: Peter Hutterer <peter.hutterer@who-t.net>
Cc: Martin Kepplinger <martink@posteo.de>
Cc: "Paul E. McKenney" <paulmck@linux.ibm.com>
---
drivers/input/misc/uinput.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 26ec603f..a3fb3b1 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -714,9 +714,15 @@ static __poll_t uinput_poll(struct file *file, poll_table *wait)
static int uinput_release(struct inode *inode, struct file *file)
{
struct uinput_device *udev = file->private_data;
+ ssize_t retval;
+
+ retval = mutex_lock_interruptible(&udev->mutex);
+ if (retval)
+ return retval;
uinput_destroy_device(udev);
kfree(udev);
+ mutex_unlock(&udev->mutex);
return 0;
}
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
^ permalink raw reply related
* [PATCH -next] Input: uinput - Avoid Use-After-Free with udev lock
From: Mukesh Ojha @ 2019-03-28 10:06 UTC (permalink / raw)
To: linux-input, linux-kernel; +Cc: Mukesh Ojha
uinput_destroy_device() gets called from two places. In one place,
uinput_ioctl_handler() it is protected under a lock udev->mutex
but same is not true for other place inside uinput_release().
This can result in a race where udev device gets freed while it
is in use.
[ 160.093398] Call trace:
[ 160.093417] kernfs_get+0x64/0x88
[ 160.093438] kernfs_new_node+0x94/0xc8
[ 160.093450] kernfs_create_dir_ns+0x44/0xfc
[ 160.093463] sysfs_create_dir_ns+0xa8/0x130
[ 160.093479] kobject_add_internal+0x278/0x650
[ 160.093491] kobject_add_varg+0xe0/0x130
[ 160.093502] kobject_add+0x15c/0x1d0
[ 160.093518] device_add+0x2bc/0xde0
[ 160.093533] input_register_device+0x5f4/0xa0c
[ 160.093547] uinput_ioctl_handler+0x1184/0x2198
[ 160.093560] uinput_ioctl+0x38/0x48
[ 160.093573] vfs_ioctl+0x7c/0xb4
[ 160.093585] do_vfs_ioctl+0x9ec/0x2350
[ 160.093597] SyS_ioctl+0x6c/0xa4
[ 160.093610] el0_svc_naked+0x34/0x38
[ 160.093621] ---[ end trace bccf0093cda2c538 ]---
[ 160.099041] =============================================================================
[ 160.107459] BUG kernfs_node_cache (Tainted: G S W O ): Object already free
[ 160.115235] -----------------------------------------------------------------------------
[ 160.115235]
[ 160.125151] Disabling lock debugging due to kernel taint
[ 160.130626] INFO: Allocated in __kernfs_new_node+0x8c/0x3c0 age=11 cpu=2 pid=7098
[ 160.138314] kmem_cache_alloc+0x358/0x388
[ 160.142445] __kernfs_new_node+0x8c/0x3c0
[ 160.146590] kernfs_new_node+0x80/0xc8
[ 160.150462] kernfs_create_dir_ns+0x44/0xfc
[ 160.154777] sysfs_create_dir_ns+0xa8/0x130
[ 160.158416] CPU5: update max cpu_capacity 1024
[ 160.159085] kobject_add_internal+0x278/0x650
[ 160.163567] kobject_add_varg+0xe0/0x130
[ 160.167606] kobject_add+0x15c/0x1d0
[ 160.168452] CPU5: update max cpu_capacity 780
[ 160.171287] get_device_parent+0x2d0/0x34c
[ 160.175510] device_add+0x240/0xde0
[ 160.178371] CPU6: update max cpu_capacity 916
[ 160.179108] input_register_device+0x5f4/0xa0c
[ 160.183686] uinput_ioctl_handler+0x1184/0x2198
[ 160.188346] uinput_ioctl+0x38/0x48
[ 160.191941] vfs_ioctl+0x7c/0xb4
[ 160.195261] do_vfs_ioctl+0x9ec/0x2350
[ 160.199111] SyS_ioctl+0x6c/0xa4
[ 160.202436] INFO: Freed in kernfs_put+0x2c8/0x434 age=14 cpu=0 pid=7096
[ 160.209230] kernfs_put+0x2c8/0x434
[ 160.212825] kobject_del+0x50/0xcc
[ 160.216332] cleanup_glue_dir+0x124/0x16c
[ 160.220456] device_del+0x55c/0x5c8
[ 160.224047] __input_unregister_device+0x274/0x2a8
[ 160.228974] input_unregister_device+0x90/0xd0
[ 160.233553] uinput_destroy_device+0x15c/0x1dc
[ 160.238131] uinput_release+0x44/0x5c
[ 160.241898] __fput+0x1f4/0x4e4
[ 160.245127] ____fput+0x20/0x2c
[ 160.248358] task_work_run+0x9c/0x174
[ 160.252127] do_notify_resume+0x104/0x6bc
[ 160.256253] work_pending+0x8/0x14
[ 160.259751] INFO: Slab 0xffffffbf0215ff00 objects=33 used=11 fp=0xffffffc0857ffd08 flags=0x8101
[ 160.268693] INFO: Object 0xffffffc0857ffd08 @offset=15624 fp=0xffffffc0857fefb0
[ 160.268693]
[ 160.277721] Redzone ffffffc0857ffd00: bb bb bb bb bb bb bb bb ........
[ 160.286656] Object ffffffc0857ffd08: 00 00 00 00 01 00 00 80 58 a2 37 45 c1 ff ff ff ........X.7E....
[ 160.296207] Object ffffffc0857ffd18: ae 21 10 0b 90 ff ff ff 20 fd 7f 85 c0 ff ff ff .!...... .......
[ 160.305780] Object ffffffc0857ffd28: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 160.315342] Object ffffffc0857ffd38: 00 00 00 00 00 00 00 00 7d a3 25 69 00 00 00 00 ........}.%i....
[ 160.324896] Object ffffffc0857ffd48: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
[ 160.334446] Object ffffffc0857ffd58: 80 c0 28 47 c1 ff ff ff 00 00 00 00 00 00 00 00 ..(G............
[ 160.344000] Object ffffffc0857ffd68: 80 4a ce d1 c0 ff ff ff dc 32 01 00 01 00 00 00 .J.......2......
[ 160.353554] Object ffffffc0857ffd78: 11 00 ed 41 00 00 00 00 00 00 00 00 00 00 00 00 ...A............
[ 160.363099] Redzone ffffffc0857ffd88: bb bb bb bb bb bb bb bb ........
[ 160.372032] Padding ffffffc0857ffee0: 5a 5a 5a 5a 5a 5a 5a 5a ZZZZZZZZ
[ 160.378299] CPU6: update max cpu_capacity 780
[ 160.380971] CPU: 4 PID: 7098 Comm: syz-executor Tainted: G S B W O 4.14.98+ #1
So, avoid the race by taking a udev lock inside `uinput_release()`.
Change-Id: I3bbb07589b7b6e0e1b3bea572b5eb4f6b09774d6
Signed-off-by: Mukesh Ojha <mojha@codeaurora.org>
Cc:Gaurav Kohli <gkohli@codeaurora.org>
Cc:Peter Hutterer <peter.hutterer@who-t.net>
Cc:Martin Kepplinger <martink@posteo.de>
Cc:"Paul E. McKenney" <paulmck@linux.ibm.com>
---
drivers/input/misc/uinput.c | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/drivers/input/misc/uinput.c b/drivers/input/misc/uinput.c
index 26ec603f..a3fb3b1 100644
--- a/drivers/input/misc/uinput.c
+++ b/drivers/input/misc/uinput.c
@@ -714,9 +714,15 @@ static __poll_t uinput_poll(struct file *file, poll_table *wait)
static int uinput_release(struct inode *inode, struct file *file)
{
struct uinput_device *udev = file->private_data;
+ ssize_t retval;
+
+ retval = mutex_lock_interruptible(&udev->mutex);
+ if (retval)
+ return retval;
uinput_destroy_device(udev);
kfree(udev);
+ mutex_unlock(&udev->mutex);
return 0;
}
--
Qualcomm India Private Limited, on behalf of Qualcomm Innovation Center,
Inc. is a member of the Code Aurora Forum, a Linux Foundation Collaborative Project
^ permalink raw reply related
* Re: [PATCH v3 2/4] lib/hexdump.c: factor out generic hexdump formatting for reuse.
From: Andy Shevchenko @ 2019-03-28 9:03 UTC (permalink / raw)
To: Life is hard, and then you die
Cc: Dmitry Torokhov, Henrik Rydberg, Sergey Senozhatsky,
Steven Rostedt, Greg Kroah-Hartman, Rafael J. Wysocki,
Lukas Wunner, Federico Lorenzi, linux-input,
Linux Kernel Mailing List
In-Reply-To: <20190328003459.GG24753@innovation.ch>
On Wed, Mar 27, 2019 at 05:34:59PM -0700, Life is hard, and then you die wrote:
>
> On Wed, Mar 27, 2019 at 09:46:48AM +0200, Andy Shevchenko wrote:
> > On Wed, Mar 27, 2019 at 3:49 AM Ronald Tschalär <ronald@innovation.ch> wrote:
> > >
> > > This introduces print_hex_dump_to_cb() which contains all the hexdump
> > > formatting minus the actual printk() call, allowing an arbitrary print
> > > function to be supplied instead. And print_hex_dump() is re-implemented
> > > using print_hex_dump_to_cb().
> > >
> > > This allows other hex-dump logging functions to be provided which call
> > > printk() differently or even log the hexdump somewhere entirely
> > > different.
> > In any case, don't do it like this. smaller non-recursive printf() is
> > better than one big receursive call.
> > When it looks like an optimization, it's actually a regression.
>
> Not sure where you see recursion here - are you referring to the
> callback approach?
%pV is a recursive printf().
> Since dev_printk() ends up calling printk with a
> dictionary as well as additional formatting, vs print_hex_dump()'s
> stright use of printk, this seemed like the best way accommodate
> various possible ways of logging the messages. But as per below I
> guess this is moot.
I recommend to read this: https://lwn.net/Articles/780556/
> > And yes, debugfs idea is not bad.
>
> So it seems like that is the consensus. As per my other response, I'll
> do this then and leave the print_hex_dump() alone.
>
> > P.S. Also check %*ph specifier.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* RE: [PATCH] Input: elan_i2c - Add i2c_reset in sysfs for ELAN touchpad recovery
From: 廖崇榮 @ 2019-03-28 7:48 UTC (permalink / raw)
To: 'Dmitry Torokhov'
Cc: linux-kernel, linux-input, ulrik.debie-os, Roger.Whittaker
In-Reply-To: <20190327043423.GD40550@dtor-ws>
Hi Dmitry,
-----Original Message-----
From: Dmitry Torokhov [mailto:dmitry.torokhov@gmail.com]
Sent: Wednesday, March 27, 2019 12:34 PM
To: KT Liao
Cc: linux-kernel@vger.kernel.org; linux-input@vger.kernel.org;
ulrik.debie-os@e2big.org; Roger.Whittaker@suse.com
Subject: Re: [PATCH] Input: elan_i2c - Add i2c_reset in sysfs for ELAN
touchpad recovery
Hi KT,
On Sat, Feb 02, 2019 at 03:54:56PM +0800, KT Liao wrote:
> Roger from SUSE reported the touchpad on Lenovo yoga2 crush sometimes.
> He found that rmmod/modprobe elan_i2c will recover the issue.
> He add the workaround on SUSE and solve the problem.
> Recently, the workaround fails in kernel 4.20 becasue IRQ mismatch.
> genirq: Flags mismatch irq 0. 00002002 (ELAN0600:00) vs. 00015a00
> (timer) I can't reproduce the issue in SUSE with the same kernel.
> And it's a 5 years old laptop, ELAN can't find the module for testing.
> Instead of IRQ debugging IRQ, I tried another approach.
> I added i2c_reset in sysfs to avoid IRQ requesting in probe.
How will users discover this flag? I do not think this is the best approach.
Can we detect that the touchpad firmware crashed from the kernel and reset
automatically?
Agree your better idea.
I will modify the driver for Roger's testing.
Will upstream after his testing.
Thanks.
>
> Signed-off-by: KT Liao <kt.liao@emc.com.tw>
> Acked-by: Roger Whittaker <Roger.Whittaker@suse.com>
> ---
> drivers/input/mouse/elan_i2c_core.c | 25 +++++++++++++++++++++++++
> 1 file changed, 25 insertions(+)
>
> diff --git a/drivers/input/mouse/elan_i2c_core.c
> b/drivers/input/mouse/elan_i2c_core.c
> index 2690a4b..388b1f0 100644
> --- a/drivers/input/mouse/elan_i2c_core.c
> +++ b/drivers/input/mouse/elan_i2c_core.c
> @@ -670,6 +670,29 @@ static ssize_t calibrate_store(struct device *dev,
> return retval ?: count;
> }
>
> +static ssize_t i2c_reset_store(struct device *dev,
> + struct device_attribute *attr,
> + const char *buf, size_t count) {
> + struct i2c_client *client = to_i2c_client(dev);
> + struct elan_tp_data *data = i2c_get_clientdata(client);
> + int retval;
> +
> + retval = mutex_lock_interruptible(&data->sysfs_mutex);
> + if (retval)
> + return retval;
> +
> + disable_irq(client->irq);
> +
> + retval = elan_initialize(data);
> + if (retval)
> + dev_err(dev, "failed to re-initialize touchpad: %d\n",
retval);
> +
> + enable_irq(client->irq);
> + mutex_unlock(&data->sysfs_mutex);
> + return retval ?: count;
> +}
> +
> static ssize_t elan_sysfs_read_mode(struct device *dev,
> struct device_attribute *attr,
> char *buf)
> @@ -702,6 +725,7 @@ static DEVICE_ATTR(mode, S_IRUGO,
> elan_sysfs_read_mode, NULL); static DEVICE_ATTR(update_fw, S_IWUSR,
> NULL, elan_sysfs_update_fw);
>
> static DEVICE_ATTR_WO(calibrate);
> +static DEVICE_ATTR_WO(i2c_reset);
>
> static struct attribute *elan_sysfs_entries[] = {
> &dev_attr_product_id.attr,
> @@ -710,6 +734,7 @@ static struct attribute *elan_sysfs_entries[] = {
> &dev_attr_iap_version.attr,
> &dev_attr_fw_checksum.attr,
> &dev_attr_calibrate.attr,
> + &dev_attr_i2c_reset.attr,
> &dev_attr_mode.attr,
> &dev_attr_update_fw.attr,
> NULL,
> --
> 2.7.4
>
--
Dmitry
^ permalink raw reply
* Re: [PATCH 1/2] Input: synaptics-rmi4 - clear irqs before set irqs
From: Aaron Ma @ 2019-03-28 6:02 UTC (permalink / raw)
To: Christopher Heiny, dmitry.torokhov@gmail.com,
linux-input@vger.kernel.org, linux-kernel@vger.kernel.org,
Andrew Duggan, benjamin.tissoires@redhat.com
In-Reply-To: <a0f21198-5715-8858-8368-51e43092181e@canonical.com>
Hi Dmitry and Chiristopher:
Do you have any suggestion about these 2 patches?
Many users confirmed that they fixed issues of Trackpoint/Touchpad after S3.
Will you consider them be accepted?
Thanks,
Aaron
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox