| 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 |
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
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.
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
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