diff mbox series

[v2,3/3] usb: misc: ljca: print firmware version

Message ID 20241106123438.337117-3-stanislaw.gruszka@linux.intel.com (mailing list archive)
State New
Headers show
Series [v2,1/3] usb: misc: ljca: move usb_autopm_put_interface() after wait for response | expand

Commit Message

Stanislaw Gruszka Nov. 6, 2024, 12:34 p.m. UTC 
For diagnostics purposes read firmware version from device
and print it to dmesg during initialization.

Reviewed-by: Hans de Goede <hdegoede@redhat.com>
Tested-by: Hans de Goede <hdegoede@redhat.com> # ThinkPad X1 Yoga Gen 8, ov2740
Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
---
 drivers/usb/misc/usb-ljca.c | 29 +++++++++++++++++++++++++++++
 1 file changed, 29 insertions(+)

Comments

Greg KH Nov. 6, 2024, 12:42 p.m. UTC | #1 
On Wed, Nov 06, 2024 at 01:34:38PM +0100, Stanislaw Gruszka wrote:
> For diagnostics purposes read firmware version from device
> and print it to dmesg during initialization.

No, sorry, when drivers work properly, they are quiet.  Think about what
your kernel log would look like if you did this for every single driver
in the tree.

> 
> Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> Tested-by: Hans de Goede <hdegoede@redhat.com> # ThinkPad X1 Yoga Gen 8, ov2740
> Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> ---
>  drivers/usb/misc/usb-ljca.c | 29 +++++++++++++++++++++++++++++
>  1 file changed, 29 insertions(+)
> 
> diff --git a/drivers/usb/misc/usb-ljca.c b/drivers/usb/misc/usb-ljca.c
> index d9c21f783055..e698a1075a40 100644
> --- a/drivers/usb/misc/usb-ljca.c
> +++ b/drivers/usb/misc/usb-ljca.c
> @@ -43,6 +43,7 @@ enum ljca_client_type {
>  
>  /* MNG client commands */
>  enum ljca_mng_cmd {
> +	LJCA_MNG_GET_VERSION = 1,
>  	LJCA_MNG_RESET = 2,
>  	LJCA_MNG_ENUM_GPIO = 4,
>  	LJCA_MNG_ENUM_I2C = 5,
> @@ -68,6 +69,13 @@ struct ljca_msg {
>  	u8 data[] __counted_by(len);
>  } __packed;
>  
> +struct ljca_fw_version {
> +	u8 major;
> +	u8 minor;
> +	__le16 patch;
> +	__le16 build;
> +} __packed;
> +
>  struct ljca_i2c_ctr_info {
>  	u8 id;
>  	u8 capacity;
> @@ -695,6 +703,25 @@ static int ljca_reset_handshake(struct ljca_adapter *adap)
>  	return 0;
>  }
>  
> +static void ljca_print_fw_version(struct ljca_adapter *adap)
> +{
> +	struct ljca_fw_version version = {};
> +	int ret;
> +
> +	ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_GET_VERSION, NULL, 0,
> +			(u8 *)&version, sizeof(version), true,
> +			LJCA_WRITE_ACK_TIMEOUT_MS);
> +
> +	if (ret != sizeof(version)) {
> +		dev_err(adap->dev, "Get version failed, ret: %d\n", ret);
> +		return;

Why not return the error?

> +	}
> +
> +	dev_info(adap->dev, "Firmware version: %d.%d.%d.%d\n",
> +		 version.major, version.minor,
> +		 le16_to_cpu(version.patch), le16_to_cpu(version.build));

Again, sorry, but no.  Feel free to dump this in a sysfs file if you
really want to get access to it, but not in the kernel log.

thanks,

greg k-h
Sakari Ailus Nov. 6, 2024, 1:37 p.m. UTC | #2 
Hi Greg,

On Wed, Nov 06, 2024 at 01:42:33PM +0100, Greg KH wrote:
> On Wed, Nov 06, 2024 at 01:34:38PM +0100, Stanislaw Gruszka wrote:
> > For diagnostics purposes read firmware version from device
> > and print it to dmesg during initialization.
> 
> No, sorry, when drivers work properly, they are quiet.  Think about what
> your kernel log would look like if you did this for every single driver
> in the tree.
> 
> > 
> > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > Tested-by: Hans de Goede <hdegoede@redhat.com> # ThinkPad X1 Yoga Gen 8, ov2740
> > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> > ---
> >  drivers/usb/misc/usb-ljca.c | 29 +++++++++++++++++++++++++++++
> >  1 file changed, 29 insertions(+)
> > 
> > diff --git a/drivers/usb/misc/usb-ljca.c b/drivers/usb/misc/usb-ljca.c
> > index d9c21f783055..e698a1075a40 100644
> > --- a/drivers/usb/misc/usb-ljca.c
> > +++ b/drivers/usb/misc/usb-ljca.c
> > @@ -43,6 +43,7 @@ enum ljca_client_type {
> >  
> >  /* MNG client commands */
> >  enum ljca_mng_cmd {
> > +	LJCA_MNG_GET_VERSION = 1,
> >  	LJCA_MNG_RESET = 2,
> >  	LJCA_MNG_ENUM_GPIO = 4,
> >  	LJCA_MNG_ENUM_I2C = 5,
> > @@ -68,6 +69,13 @@ struct ljca_msg {
> >  	u8 data[] __counted_by(len);
> >  } __packed;
> >  
> > +struct ljca_fw_version {
> > +	u8 major;
> > +	u8 minor;
> > +	__le16 patch;
> > +	__le16 build;
> > +} __packed;
> > +
> >  struct ljca_i2c_ctr_info {
> >  	u8 id;
> >  	u8 capacity;
> > @@ -695,6 +703,25 @@ static int ljca_reset_handshake(struct ljca_adapter *adap)
> >  	return 0;
> >  }
> >  
> > +static void ljca_print_fw_version(struct ljca_adapter *adap)
> > +{
> > +	struct ljca_fw_version version = {};
> > +	int ret;
> > +
> > +	ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_GET_VERSION, NULL, 0,
> > +			(u8 *)&version, sizeof(version), true,
> > +			LJCA_WRITE_ACK_TIMEOUT_MS);
> > +
> > +	if (ret != sizeof(version)) {
> > +		dev_err(adap->dev, "Get version failed, ret: %d\n", ret);
> > +		return;
> 
> Why not return the error?

An error here would indicate something is a little fishy but doesn't
prevent the device from working as such. I'd think it's fine as-is.

> 
> > +	}
> > +
> > +	dev_info(adap->dev, "Firmware version: %d.%d.%d.%d\n",
> > +		 version.major, version.minor,
> > +		 le16_to_cpu(version.patch), le16_to_cpu(version.build));
> 
> Again, sorry, but no.  Feel free to dump this in a sysfs file if you
> really want to get access to it, but not in the kernel log.

I guess dev_dbg() should do as well.
Stanislaw Gruszka Nov. 6, 2024, 2:50 p.m. UTC | #3 
On Wed, Nov 06, 2024 at 01:37:58PM +0000, Sakari Ailus wrote:
> Hi Greg,
> 
> On Wed, Nov 06, 2024 at 01:42:33PM +0100, Greg KH wrote:
> > On Wed, Nov 06, 2024 at 01:34:38PM +0100, Stanislaw Gruszka wrote:
> > > For diagnostics purposes read firmware version from device
> > > and print it to dmesg during initialization.
> > 
> > No, sorry, when drivers work properly, they are quiet.  Think about what
> > your kernel log would look like if you did this for every single driver
> > in the tree.

Not single one, but there are plenty of drivers in the tree that
print driver/firmware/hardware version to the log. Few in the usb
subsystem:

drivers/usb/fotg210/fotg210-udc.c:      dev_info(dev, "version %s\n", DRIVER_VERSION);
drivers/usb/gadget/legacy/acm_ms.c:     dev_info(&gadget->dev, "%s, version: " DRIVER_VERSION "\n",
drivers/usb/gadget/legacy/cdc2.c:       dev_info(&gadget->dev, "%s, version: " DRIVER_VERSION "\n",
drivers/usb/gadget/legacy/ether.c:      dev_info(&gadget->dev, "%s, version: " DRIVER_VERSION "\n",
drivers/usb/gadget/legacy/hid.c:        dev_info(&gadget->dev, DRIVER_DESC ", version: " DRIVER_VERSION "\n");
drivers/usb/gadget/udc/fusb300_udc.c:   dev_info(&pdev->dev, "version %s\n", DRIVER_VERSION);
drivers/usb/gadget/udc/lpc32xx_udc.c:   dev_info(udc->dev, "%s version %s\n", driver_name, DRIVER_VERSION);
drivers/usb/gadget/udc/m66592-udc.c:    dev_info(&pdev->dev, "version %s\n", DRIVER_VERSION);
drivers/usb/gadget/udc/net2272.c:       dev_info(dev->dev, "version: %s\n", driver_vers);
drivers/usb/gadget/udc/net2272.c:       dev_info(dev->dev, "RDK2 FPGA version %08x\n",
drivers/usb/gadget/udc/r8a66597-udc.c:  dev_info(dev, "version %s\n", DRIVER_VERSION);
drivers/usb/gadget/udc/renesas_usbf.c:  dev_info(dev, "USBF version: %08x\n",
drivers/usb/host/xhci-mtk.c:    dev_info(mtk->dev, "uwk - reg:0x%x, version:%d\n",
drivers/usb/mtu3/mtu3_core.c:   dev_info(mtu->dev, "IP version 0x%x(%s IP)\n", mtu->hw_version,
drivers/usb/mtu3/mtu3_host.c:   dev_info(ssusb->dev, "uwk - reg:0x%x, version:%d\n",
drivers/usb/typec/ucsi/ucsi_ccg.c:              dev_info(dev, "secondary fw version is too low (< %d)\n",
drivers/usb/typec/ucsi/ucsi_ccg.c:              dev_info(dev, "found primary fw with later version\n");
drivers/usb/typec/ucsi/ucsi_stm32g0.c:          dev_info(g0->dev, "Flashing FW: %08x (%08x cur)\n", fw_info->version, fw_version);
drivers/usb/typec/ucsi/ucsi_stm32g0.c:  dev_info(g0->dev, "Bootloader Version 0x%02x\n", g0->bl_version);

> > > Reviewed-by: Hans de Goede <hdegoede@redhat.com>
> > > Tested-by: Hans de Goede <hdegoede@redhat.com> # ThinkPad X1 Yoga Gen 8, ov2740
> > > Signed-off-by: Stanislaw Gruszka <stanislaw.gruszka@linux.intel.com>
> > > ---
> > >  drivers/usb/misc/usb-ljca.c | 29 +++++++++++++++++++++++++++++
<snip>

> > > +	}
> > > +
> > > +	dev_info(adap->dev, "Firmware version: %d.%d.%d.%d\n",
> > > +		 version.major, version.minor,
> > > +		 le16_to_cpu(version.patch), le16_to_cpu(version.build));
> > 
> > Again, sorry, but no.  Feel free to dump this in a sysfs file if you
> > really want to get access to it, but not in the kernel log.
> 
> I guess dev_dbg() should do as well.

I think that's better as we currently do not support sysfs in ljca,
so extra code would be needed.

Regards
Stanislaw
Greg KH Nov. 7, 2024, 4:21 a.m. UTC | #4 
On Wed, Nov 06, 2024 at 03:50:59PM +0100, Stanislaw Gruszka wrote:
> On Wed, Nov 06, 2024 at 01:37:58PM +0000, Sakari Ailus wrote:
> > Hi Greg,
> > 
> > On Wed, Nov 06, 2024 at 01:42:33PM +0100, Greg KH wrote:
> > > On Wed, Nov 06, 2024 at 01:34:38PM +0100, Stanislaw Gruszka wrote:
> > > > For diagnostics purposes read firmware version from device
> > > > and print it to dmesg during initialization.
> > > 
> > > No, sorry, when drivers work properly, they are quiet.  Think about what
> > > your kernel log would look like if you did this for every single driver
> > > in the tree.
> 
> Not single one, but there are plenty of drivers in the tree that
> print driver/firmware/hardware version to the log. Few in the usb
> subsystem:
> 
> drivers/usb/fotg210/fotg210-udc.c:      dev_info(dev, "version %s\n", DRIVER_VERSION);
> drivers/usb/gadget/legacy/acm_ms.c:     dev_info(&gadget->dev, "%s, version: " DRIVER_VERSION "\n",
> drivers/usb/gadget/legacy/cdc2.c:       dev_info(&gadget->dev, "%s, version: " DRIVER_VERSION "\n",
> drivers/usb/gadget/legacy/ether.c:      dev_info(&gadget->dev, "%s, version: " DRIVER_VERSION "\n",
> drivers/usb/gadget/legacy/hid.c:        dev_info(&gadget->dev, DRIVER_DESC ", version: " DRIVER_VERSION "\n");
> drivers/usb/gadget/udc/fusb300_udc.c:   dev_info(&pdev->dev, "version %s\n", DRIVER_VERSION);
> drivers/usb/gadget/udc/lpc32xx_udc.c:   dev_info(udc->dev, "%s version %s\n", driver_name, DRIVER_VERSION);
> drivers/usb/gadget/udc/m66592-udc.c:    dev_info(&pdev->dev, "version %s\n", DRIVER_VERSION);
> drivers/usb/gadget/udc/net2272.c:       dev_info(dev->dev, "version: %s\n", driver_vers);
> drivers/usb/gadget/udc/net2272.c:       dev_info(dev->dev, "RDK2 FPGA version %08x\n",
> drivers/usb/gadget/udc/r8a66597-udc.c:  dev_info(dev, "version %s\n", DRIVER_VERSION);
> drivers/usb/gadget/udc/renesas_usbf.c:  dev_info(dev, "USBF version: %08x\n",
> drivers/usb/host/xhci-mtk.c:    dev_info(mtk->dev, "uwk - reg:0x%x, version:%d\n",
> drivers/usb/mtu3/mtu3_core.c:   dev_info(mtu->dev, "IP version 0x%x(%s IP)\n", mtu->hw_version,
> drivers/usb/mtu3/mtu3_host.c:   dev_info(ssusb->dev, "uwk - reg:0x%x, version:%d\n",
> drivers/usb/typec/ucsi/ucsi_ccg.c:              dev_info(dev, "secondary fw version is too low (< %d)\n",
> drivers/usb/typec/ucsi/ucsi_ccg.c:              dev_info(dev, "found primary fw with later version\n");
> drivers/usb/typec/ucsi/ucsi_stm32g0.c:          dev_info(g0->dev, "Flashing FW: %08x (%08x cur)\n", fw_info->version, fw_version);
> drivers/usb/typec/ucsi/ucsi_stm32g0.c:  dev_info(g0->dev, "Bootloader Version 0x%02x\n", g0->bl_version);

Yes, and all of those should be fixed up and removed.

Also, the idea of "DRIVER_VERSION" is obsolete for a very very long time
and should also just be removed entirely.  We swept it from many drivers
years ago, looks like it snuck back in or that we missed some.

thanks,

greg k-h
diff mbox series

Patch

diff --git a/drivers/usb/misc/usb-ljca.c b/drivers/usb/misc/usb-ljca.c
index d9c21f783055..e698a1075a40 100644
--- a/drivers/usb/misc/usb-ljca.c
+++ b/drivers/usb/misc/usb-ljca.c
@@ -43,6 +43,7 @@  enum ljca_client_type {
 
 /* MNG client commands */
 enum ljca_mng_cmd {
+	LJCA_MNG_GET_VERSION = 1,
 	LJCA_MNG_RESET = 2,
 	LJCA_MNG_ENUM_GPIO = 4,
 	LJCA_MNG_ENUM_I2C = 5,
@@ -68,6 +69,13 @@  struct ljca_msg {
 	u8 data[] __counted_by(len);
 } __packed;
 
+struct ljca_fw_version {
+	u8 major;
+	u8 minor;
+	__le16 patch;
+	__le16 build;
+} __packed;
+
 struct ljca_i2c_ctr_info {
 	u8 id;
 	u8 capacity;
@@ -695,6 +703,25 @@  static int ljca_reset_handshake(struct ljca_adapter *adap)
 	return 0;
 }
 
+static void ljca_print_fw_version(struct ljca_adapter *adap)
+{
+	struct ljca_fw_version version = {};
+	int ret;
+
+	ret = ljca_send(adap, LJCA_CLIENT_MNG, LJCA_MNG_GET_VERSION, NULL, 0,
+			(u8 *)&version, sizeof(version), true,
+			LJCA_WRITE_ACK_TIMEOUT_MS);
+
+	if (ret != sizeof(version)) {
+		dev_err(adap->dev, "Get version failed, ret: %d\n", ret);
+		return;
+	}
+
+	dev_info(adap->dev, "Firmware version: %d.%d.%d.%d\n",
+		 version.major, version.minor,
+		 le16_to_cpu(version.patch), le16_to_cpu(version.build));
+}
+
 static int ljca_enumerate_clients(struct ljca_adapter *adap)
 {
 	struct ljca_client *client, *next;
@@ -811,6 +838,8 @@  static int ljca_probe(struct usb_interface *interface,
 	if (ret)
 		goto err_free;
 
+	ljca_print_fw_version(adap);
+
 	/*
 	 * This works around problems with ov2740 initialization on some
 	 * Lenovo platforms. The autosuspend delay, has to be smaller than