| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <ZpmPvaknDw2cGnOQ@google.com>
Date: Thu, 18 Jul 2024 21:57:17 +0000
From: Matthias Kaehlcke <mka@...omium.org>
To: Radhey Shyam Pandey <radhey.shyam.pandey@....com>
Cc: gregkh@...uxfoundation.org, javier.carrasco@...fvision.net,
benjamin.bara@...data.com, macpaul.lin@...iatek.com,
linux.amoon@...il.com, linux-usb@...r.kernel.org,
linux-kernel@...r.kernel.org, git@....com
Subject: Re: [PATCH v2 2/2] usb: misc: onboard_usb_dev: add Microchip usb5744
SMBus programming support
Hi,
On Thu, Jul 18, 2024 at 12:53:43AM +0530, Radhey Shyam Pandey wrote:
> usb5744 supports SMBus Configuration and it may be configured via the
> SMBus slave interface during the hub start-up configuration stage.
>
> To program it driver uses i2c-bus phandle (added in commit '02be19e914b8
> dt-bindings: usb: Add support for Microchip usb5744 hub controller') to
> get i2c client device and then based on usb5744 compatible check calls
> usb5744 i2c default initialization sequence.
>
> Apart from the USB command attach, prevent the hub from suspend.
> when the USB Attach with SMBus (0xAA56) command is issued to the hub,
> the hub is getting enumerated and then it puts in a suspend mode.
> This causes the hub to NAK any SMBus access made by the SMBus Master
> during this period and not able to see the hub's slave address while
> running the "i2c probe" command.
>
> Prevent the MCU from the putting the HUB in suspend mode through
> register write. The BYPASS_UDC_SUSPEND bit (Bit 3) of the RuntimeFlags2
> register at address 0x411D controls this aspect of the hub. The
> BYPASS_UDC_SUSPEND bit in register 0x411Dh must be set to ensure that the
> MCU is always enabled and ready to respond to SMBus runtime commands.
> This register needs to be written before the USB attach command is issued.
>
> The byte sequence is as follows:
> Slave addr: 0x2d 00 00 05 00 01 41 1D 08
> Slave addr: 0x2d 99 37 00
> Slave addr: 0x2d AA 56 00
>
> Signed-off-by: Radhey Shyam Pandey <radhey.shyam.pandey@....com>
> ---
> Changes for v2:
> - Move power on reset delay to separate patch.
> - Switch to compatible based check for calling usb5755
> onboard_dev_5744_i2c_init(). This is done to make
> onboard_dev_5744_i2c_init() as static.
> - Fix subsystem "usb: misc: onboard_usb_dev:..."
> - Use #define for different register bits instead of magic values.
> - Use err_power_off label name.
> - Modified commit description to be in sync with v2 changes.
> ---
> drivers/usb/misc/onboard_usb_dev.c | 56 ++++++++++++++++++++++++++++++
> drivers/usb/misc/onboard_usb_dev.h | 12 +++++++
> 2 files changed, 68 insertions(+)
>
> diff --git a/drivers/usb/misc/onboard_usb_dev.c b/drivers/usb/misc/onboard_usb_dev.c
> index 94d5424841fd..4f3845f35ac4 100644
> --- a/drivers/usb/misc/onboard_usb_dev.c
> +++ b/drivers/usb/misc/onboard_usb_dev.c
> @@ -11,6 +11,7 @@
> #include <linux/err.h>
> #include <linux/gpio/consumer.h>
> #include <linux/init.h>
> +#include <linux/i2c.h>
> #include <linux/kernel.h>
> #include <linux/list.h>
> #include <linux/module.h>
> @@ -297,10 +298,40 @@ static void onboard_dev_attach_usb_driver(struct work_struct *work)
> pr_err("Failed to attach USB driver: %pe\n", ERR_PTR(err));
> }
>
> +static int onboard_dev_5744_i2c_init(struct i2c_client *client)
> +{
> + struct device *dev = &client->dev;
> + int ret;
> +
Please explain what the purpose of this sequence is. It is described
in the commit message, however that is not directly available to a
reader of the code.
> + char wr_buf[7] = {USB5744_CREG_MEM_ADDR, USB5744_CREG_MEM_NBYTES,
IIUC USB5744_CREG_MEM_NBYTES is the number of registers written to the
configuration register by this specific command, it could be different
for other commands. If that is correct then I think this shouldn't be
a constant but a literal plus a comment.
> + USB5744_CREG_WRITE, USB5744_CREG_NBYTES,
Similar as above, USB5744_CREG_NBYTES should be a literal + comment, or
alternatively something like USB5744_CREG_SIZE, assuming the width of
all registers is one byte.
It would also be an option to add a wrapper onboard_dev_5744_write_creg(),
that encapsulates writing a configuration register. Not strictly need
since currently only one register is written.
> + USB5744_CREG_RUNTIMEFLAGS2,
> + USB5744_CREG_RUNTIMEFLAGS2_LSB,
> + USB5744_CREG_BYPASS_UDC_SUSPEND};
> +
> + ret = i2c_smbus_write_block_data(client, 0, sizeof(wr_buf), wr_buf);
> + if (ret)
> + return dev_err_probe(dev, ret, "BYPASS_UDC_SUSPEND bit configuration failed\n");
> +
> + ret = i2c_smbus_write_word_data(client, USB5744_CREG_ACCESS,
> + USB5744_CREG_ACCESS_LSB);
This is a command ("Configuration Register Access command", this should be
reflected in the name, as for USB5744_CMD_ATTACH.
> + if (ret)
> + return dev_err_probe(dev, ret, "Configuration Register Access Command failed\n");
> +
> + /* Send SMBus command to boot hub. */
> + ret = i2c_smbus_write_word_data(client, USB5744_CMD_ATTACH,
> + USB5744_CMD_ATTACH_LSB);
> + if (ret < 0)
> + return dev_err_probe(dev, ret, "USB Attach with SMBus command failed\n");
> +
> + return ret;
return 0;
> +}
> +
> static int onboard_dev_probe(struct platform_device *pdev)
> {
> struct device *dev = &pdev->dev;
> struct onboard_dev *onboard_dev;
> + struct device_node *i2c_node;
> int err;
>
> onboard_dev = devm_kzalloc(dev, sizeof(*onboard_dev), GFP_KERNEL);
> @@ -340,6 +371,27 @@ static int onboard_dev_probe(struct platform_device *pdev)
> if (err)
> return err;
>
> + i2c_node = of_parse_phandle(pdev->dev.of_node, "i2c-bus", 0);
> + if (i2c_node) {
> + struct i2c_client *client;
> +
> + client = of_find_i2c_device_by_node(i2c_node);
> + of_node_put(i2c_node);
> +
> + if (!client) {
> + err = -EPROBE_DEFER;
> + goto err_power_off;
> + }
> +
> + if (of_device_is_compatible(pdev->dev.of_node, "usb424,2744") ||
> + of_device_is_compatible(pdev->dev.of_node, "usb424,5744"))
> + err = onboard_dev_5744_i2c_init(client);
> +
> + put_device(&client->dev);
> + if (err < 0)
> + goto err_power_off;
> + }
> +
> /*
> * The USB driver might have been detached from the USB devices by
> * onboard_dev_remove() (e.g. through an 'unbind' by userspace),
> @@ -351,6 +403,10 @@ static int onboard_dev_probe(struct platform_device *pdev)
> schedule_work(&attach_usb_driver_work);
>
> return 0;
> +
> +err_power_off:
> + onboard_dev_power_off(onboard_dev);
> + return err;
> }
>
> static void onboard_dev_remove(struct platform_device *pdev)
> diff --git a/drivers/usb/misc/onboard_usb_dev.h b/drivers/usb/misc/onboard_usb_dev.h
> index 82c76a0b3346..5744c589b90f 100644
> --- a/drivers/usb/misc/onboard_usb_dev.h
> +++ b/drivers/usb/misc/onboard_usb_dev.h
> @@ -8,6 +8,18 @@
>
> #define MAX_SUPPLIES 2
>
> +#define USB5744_CMD_ATTACH 0xAA
> +#define USB5744_CMD_ATTACH_LSB 0x56
> +#define USB5744_CREG_ACCESS 0x99
> +#define USB5744_CREG_ACCESS_LSB 0x37
> +#define USB5744_CREG_MEM_ADDR 0x00
> +#define USB5744_CREG_MEM_NBYTES 0x05
> +#define USB5744_CREG_WRITE 0x00
> +#define USB5744_CREG_NBYTES 0x01
> +#define USB5744_CREG_RUNTIMEFLAGS2 0x41
> +#define USB5744_CREG_RUNTIMEFLAGS2_LSB 0x1D
> +#define USB5744_CREG_BYPASS_UDC_SUSPEND BIT(3)
> +
These defines are specific to the Microchip 5744 hub, there is no need
for them to be defined in the include of the onboard_usb_dev driver.
Please move them to the .c file
Thanks
Matthias
Powered by blists - more mailing lists