| 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: <20230207002447.nh6hppnr4vawichj@synopsys.com>
Date: Tue, 7 Feb 2023 00:24:56 +0000
From: Thinh Nguyen <Thinh.Nguyen@...opsys.com>
To: Elson Roy Serrao <quic_eserrao@...cinc.com>
CC: "gregkh@...uxfoundation.org" <gregkh@...uxfoundation.org>,
Thinh Nguyen <Thinh.Nguyen@...opsys.com>,
"balbi@...nel.org" <balbi@...nel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"quic_wcheng@...cinc.com" <quic_wcheng@...cinc.com>,
"quic_jackp@...cinc.com" <quic_jackp@...cinc.com>
Subject: Re: [PATCH v3 1/5] usb: gadget: Properly configure the device for
remote wakeup
Hi Elson,
On Mon, Feb 06, 2023, Elson Roy Serrao wrote:
> The wakeup bit in the bmAttributes field indicates whether the device
> is configured for remote wakeup. But this field should be allowed to
> set only if the UDC supports such wakeup mechanism. So configure this
> field based on UDC capability. Also inform the UDC whether the device
> is configured for remote wakeup by implementing a gadget op.
>
> Signed-off-by: Elson Roy Serrao <quic_eserrao@...cinc.com>
> ---
> drivers/usb/gadget/composite.c | 24 +++++++++++++++++++++++-
> drivers/usb/gadget/udc/core.c | 27 +++++++++++++++++++++++++++
> drivers/usb/gadget/udc/trace.h | 5 +++++
> include/linux/usb/gadget.h | 8 ++++++++
> 4 files changed, 63 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/usb/gadget/composite.c b/drivers/usb/gadget/composite.c
> index fa7dd6c..e459fb0 100644
> --- a/drivers/usb/gadget/composite.c
> +++ b/drivers/usb/gadget/composite.c
> @@ -513,6 +513,19 @@ static u8 encode_bMaxPower(enum usb_device_speed speed,
> return min(val, 900U) / 8;
> }
>
> +static void check_remote_wakeup_config(struct usb_gadget *gadget,
> + struct usb_configuration *c)
> +{
> + if (USB_CONFIG_ATT_WAKEUP & c->bmAttributes) {
> + /* Reset the rw bit if gadget is not capable of it */
> + if (!gadget->rw_capable) {
Probably it's better to make sure we don't break the configuration for
other UDCs until this is implemented in their drivers. Let's add another
condition:
if (!gadget->rw_capable && gadget->ops->set_remotewakeup) {
...
}
> + INFO(c->cdev, "Clearing rw bit for config c.%d\n",
> + c->bConfigurationValue);
Perhaps a warning is better since we're overwriting the user's
configuration.
> + c->bmAttributes &= ~USB_CONFIG_ATT_WAKEUP;
> + }
> + }
> +}
> +
> static int config_buf(struct usb_configuration *config,
> enum usb_device_speed speed, void *buf, u8 type)
> {
> @@ -620,8 +633,12 @@ static int config_desc(struct usb_composite_dev *cdev, unsigned w_value)
> continue;
> }
>
> - if (w_value == 0)
> + if (w_value == 0) {
> + /* Correctly configure the bmAttributes wakeup bit */
> + check_remote_wakeup_config(gadget, c);
Checking here is too late. We should check in configfs_composite_bind().
> +
> return config_buf(c, speed, cdev->req->buf, type);
> + }
> w_value--;
> }
> return -EINVAL;
> @@ -1000,6 +1017,11 @@ static int set_config(struct usb_composite_dev *cdev,
> else
> usb_gadget_clear_selfpowered(gadget);
>
> + if (USB_CONFIG_ATT_WAKEUP & c->bmAttributes)
> + usb_gadget_set_remotewakeup(gadget, 1);
> + else
> + usb_gadget_set_remotewakeup(gadget, 0);
> +
> usb_gadget_vbus_draw(gadget, power);
> if (result >= 0 && cdev->delayed_status)
> result = USB_GADGET_DELAYED_STATUS;
> diff --git a/drivers/usb/gadget/udc/core.c b/drivers/usb/gadget/udc/core.c
> index 23b0629..5874d4f 100644
> --- a/drivers/usb/gadget/udc/core.c
> +++ b/drivers/usb/gadget/udc/core.c
> @@ -514,6 +514,33 @@ int usb_gadget_wakeup(struct usb_gadget *gadget)
> EXPORT_SYMBOL_GPL(usb_gadget_wakeup);
>
> /**
> + * usb_gadget_set_remotewakeup - configures the device remote wakeup feature.
> + * @gadget:the device being configured for remote wakeup
> + * @set:value to be configured.
> + *
> + * set to one to enable remote wakeup feature and zero to disable it.
> + *
> + * returns zero on success, else negative errno.
> + */
> +int usb_gadget_set_remotewakeup(struct usb_gadget *gadget, int set)
> +{
> + int ret = 0;
> +
> + if (!gadget->ops->set_remotewakeup) {
> + ret = -EOPNOTSUPP;
> + goto out;
> + }
> +
> + ret = gadget->ops->set_remotewakeup(gadget, set);
> +
> +out:
> + trace_usb_gadget_set_remotewakeup(gadget, ret);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(usb_gadget_set_remotewakeup);
> +
> +/**
> * usb_gadget_set_selfpowered - sets the device selfpowered feature.
> * @gadget:the device being declared as self-powered
> *
> diff --git a/drivers/usb/gadget/udc/trace.h b/drivers/usb/gadget/udc/trace.h
> index abdbcb1..a3314ce 100644
> --- a/drivers/usb/gadget/udc/trace.h
> +++ b/drivers/usb/gadget/udc/trace.h
> @@ -91,6 +91,11 @@ DEFINE_EVENT(udc_log_gadget, usb_gadget_wakeup,
> TP_ARGS(g, ret)
> );
>
> +DEFINE_EVENT(udc_log_gadget, usb_gadget_set_remotewakeup,
> + TP_PROTO(struct usb_gadget *g, int ret),
> + TP_ARGS(g, ret)
> +);
> +
> DEFINE_EVENT(udc_log_gadget, usb_gadget_set_selfpowered,
> TP_PROTO(struct usb_gadget *g, int ret),
> TP_ARGS(g, ret)
> diff --git a/include/linux/usb/gadget.h b/include/linux/usb/gadget.h
> index dc3092c..05d1449 100644
> --- a/include/linux/usb/gadget.h
> +++ b/include/linux/usb/gadget.h
> @@ -309,6 +309,7 @@ struct usb_udc;
> struct usb_gadget_ops {
> int (*get_frame)(struct usb_gadget *);
> int (*wakeup)(struct usb_gadget *);
> + int (*set_remotewakeup)(struct usb_gadget *, int set);
minor nit: can we change this name to set_remote_wakeup. It's a bit
easier to read IMO.
> int (*set_selfpowered) (struct usb_gadget *, int is_selfpowered);
> int (*vbus_session) (struct usb_gadget *, int is_active);
> int (*vbus_draw) (struct usb_gadget *, unsigned mA);
> @@ -383,6 +384,8 @@ struct usb_gadget_ops {
> * @connected: True if gadget is connected.
> * @lpm_capable: If the gadget max_speed is FULL or HIGH, this flag
> * indicates that it supports LPM as per the LPM ECN & errata.
> + * @rw_capable: True if gadget is capable of sending remote wakeup.
> + * @rw_armed: True if gadget is armed by the host for remote wakeup.
> * @irq: the interrupt number for device controller.
> * @id_number: a unique ID number for ensuring that gadget names are distinct
> *
> @@ -444,6 +447,8 @@ struct usb_gadget {
> unsigned deactivated:1;
> unsigned connected:1;
> unsigned lpm_capable:1;
> + unsigned rw_capable:1;
> + unsigned rw_armed:1;
> int irq;
> int id_number;
> };
> @@ -600,6 +605,7 @@ static inline int gadget_is_otg(struct usb_gadget *g)
> #if IS_ENABLED(CONFIG_USB_GADGET)
> int usb_gadget_frame_number(struct usb_gadget *gadget);
> int usb_gadget_wakeup(struct usb_gadget *gadget);
> +int usb_gadget_set_remotewakeup(struct usb_gadget *gadget, int set);
> int usb_gadget_set_selfpowered(struct usb_gadget *gadget);
> int usb_gadget_clear_selfpowered(struct usb_gadget *gadget);
> int usb_gadget_vbus_connect(struct usb_gadget *gadget);
> @@ -615,6 +621,8 @@ static inline int usb_gadget_frame_number(struct usb_gadget *gadget)
> { return 0; }
> static inline int usb_gadget_wakeup(struct usb_gadget *gadget)
> { return 0; }
> +static inline int usb_gadget_set_remotewakeup(struct usb_gadget *gadget, int set)
> +{ return 0; }
> static inline int usb_gadget_set_selfpowered(struct usb_gadget *gadget)
> { return 0; }
> static inline int usb_gadget_clear_selfpowered(struct usb_gadget *gadget)
> --
> 2.7.4
>
Thanks,
Thinh
Powered by blists - more mailing lists