[PATCH] usb: misc: cypress_cy7c63: check control transfer status

[PATCH] usb: misc: cypress_cy7c63: check control transfer status

[Keshav Verma]

read_port() currently ignores errors from vendor_command() and always
returns the cached port value. This can report stale data when the USB
control transfer fails or returns a short response.

Return the underlying error for failed transfers and report -EIO if the
device returns fewer bytes than required.

Fixes: 9189bfc2df0f ("[PATCH] USB: rename Cypress CY7C63xxx driver to proper name and fix up some tiny things")
Signed-off-by: Keshav Verma <iganschel@gmail.com>
---
 drivers/usb/misc/cypress_cy7c63.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/usb/misc/cypress_cy7c63.c b/drivers/usb/misc/cypress_cy7c63.c
index 4a7f955ba85b..8601ee0a1ea3 100644
--- a/drivers/usb/misc/cypress_cy7c63.c
+++ b/drivers/usb/misc/cypress_cy7c63.c
@@ -177,6 +177,10 @@ static ssize_t read_port(struct device *dev, struct device_attribute *attr,
 	result = vendor_command(cyp, CYPRESS_READ_PORT, read_id, 0);
 
 	dev_dbg(&cyp->udev->dev, "Result of vendor_command: %d\n\n", result);
+	if (result < 0)
+		return result;
+	if (result < 2)
+		return -EIO;
 
 	return sprintf(buf, "%d", cyp->port[port_num]);
 }
-- 
2.39.5

Re: [PATCH] usb: misc: cypress_cy7c63: check control transfer status

[Greg KH]

On Fri, Jun 19, 2026 at 02:00:53AM +0530, Keshav Verma wrote:
> read_port() currently ignores errors from vendor_command() and always
> returns the cached port value. This can report stale data when the USB
> control transfer fails or returns a short response.
> 
> Return the underlying error for failed transfers and report -EIO if the
> device returns fewer bytes than required.
> 
> Fixes: 9189bfc2df0f ("[PATCH] USB: rename Cypress CY7C63xxx driver to proper name and fix up some tiny things")
> Signed-off-by: Keshav Verma <iganschel@gmail.com>
> ---
>  drivers/usb/misc/cypress_cy7c63.c | 4 ++++
>  1 file changed, 4 insertions(+)
> 
> diff --git a/drivers/usb/misc/cypress_cy7c63.c b/drivers/usb/misc/cypress_cy7c63.c
> index 4a7f955ba85b..8601ee0a1ea3 100644
> --- a/drivers/usb/misc/cypress_cy7c63.c
> +++ b/drivers/usb/misc/cypress_cy7c63.c
> @@ -177,6 +177,10 @@ static ssize_t read_port(struct device *dev, struct device_attribute *attr,
>  	result = vendor_command(cyp, CYPRESS_READ_PORT, read_id, 0);
>  
>  	dev_dbg(&cyp->udev->dev, "Result of vendor_command: %d\n\n", result);
> +	if (result < 0)
> +		return result;
> +	if (result < 2)
> +		return -EIO;
>  
>  	return sprintf(buf, "%d", cyp->port[port_num]);
>  }

You only changed one call to vendor_command() here, why?  Why not fix
this all up by changing vendor_command() to use a better function that
will give you a sane error value, and then check that?

Start with this patch:


diff --git a/drivers/usb/misc/cytherm.c b/drivers/usb/misc/cytherm.c
index d26f460ecdbc..86c4678e23b3 100644
--- a/drivers/usb/misc/cytherm.c
+++ b/drivers/usb/misc/cytherm.c
@@ -51,12 +51,10 @@ static int vendor_command(struct usb_device *dev, unsigned char request,
 			  unsigned char value, unsigned char index,
 			  void *buf, int size)
 {
-	return usb_control_msg(dev, usb_rcvctrlpipe(dev, 0),
-			       request, 
-			       USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_OTHER,
-			       value, 
-			       index, buf, size,
-			       USB_CTRL_GET_TIMEOUT);
+	return usb_control_msg_recv(dev, 0, request,
+				    USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_OTHER,
+				    value, index, buf, size,
+				    USB_CTRL_GET_TIMEOUT, GFP_KERNEL);
 }
 

And go from there!

thanks,

greg k-h

Re: [PATCH] usb: misc: cypress_cy7c63: check control transfer status

[Keshav Verma]

Hi Greg,

Thanks for the review.
I'll rework the patch as suggested.

I may be a couple of days before I can send the updated patch, but
I'll get a revised version posted as soon as I can.

Thanks,
Keshav

Re: [PATCH] usb: misc: cypress_cy7c63: check control transfer status

[Keshav Verma]

read_port() currently ignores errors from vendor_command() and always
returns the cached port value. This can report stale data when the USB
control transfer fails or returns a short response.

Convert vendor_command() to usb_control_msg_recv(), which returns 0 on
success and a negative errno on failure, and propagate transport errors
from read_port() instead of returning cached data.

Fixes: 9189bfc2df0f ("[PATCH] USB: rename Cypress CY7C63xxx driver to
proper name and fix up some tiny things")
Signed-off-by: Keshav Verma <iganschel@gmail.com>
---
v2:
- Use usb_control_msg_recv() in vendor_command().
- Remove the obsolete short-transfer check.

 drivers/usb/misc/cypress_cy7c63.c | 27 +++++++++++++++------------
 1 file changed, 15 insertions(+), 12 deletions(-)

diff --git a/drivers/usb/misc/cypress_cy7c63.c
b/drivers/usb/misc/cypress_cy7c63.c
index 4a7f955ba85b..47cb05c3a870 100644
--- a/drivers/usb/misc/cypress_cy7c63.c
+++ b/drivers/usb/misc/cypress_cy7c63.c
@@ -69,8 +69,7 @@ struct cypress {
 static int vendor_command(struct cypress *dev, unsigned char request,
                          unsigned char address, unsigned char data)
 {
-       int retval = 0;
-       unsigned int pipe;
+       int retval;
        unsigned char *iobuf;

        /* allocate some memory for the i/o buffer*/
@@ -80,17 +79,19 @@ static int vendor_command(struct cypress *dev,
unsigned char request,
                goto error;
        }

-       dev_dbg(&dev->udev->dev, "Sending usb_control_msg (data: %d)\n", data);
-
-       /* prepare usb control message and send it upstream */
-       pipe = usb_rcvctrlpipe(dev->udev, 0);
-       retval = usb_control_msg(dev->udev, pipe, request,
-                                USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_OTHER,
-                                address, data, iobuf, CYPRESS_MAX_REQSIZE,
-                                USB_CTRL_GET_TIMEOUT);
-       /* we must not process garbage */
-       if (retval < 2)
+       dev_dbg(&dev->udev->dev, "Sending usb_control_msg_recv (data:
%d)\n", data);
+
+       retval = usb_control_msg_recv(dev->udev, 0, request,
+                               USB_DIR_IN | USB_TYPE_VENDOR |
+                               USB_RECIP_OTHER,
+                               address, data, iobuf,
+                               CYPRESS_MAX_REQSIZE,
+                               USB_CTRL_GET_TIMEOUT,
+                               GFP_KERNEL);
+       if (retval < 0) {
+               dev_err(&dev->udev->dev, "usb_control_msg_recv failed:
%d\n", retval);
                goto err_buf;
+       }

        /* store returned data (more READs to be added) */
        switch (request) {
@@ -177,6 +178,8 @@ static ssize_t read_port(struct device *dev,
struct device_attribute *attr,
        result = vendor_command(cyp, CYPRESS_READ_PORT, read_id, 0);

        dev_dbg(&cyp->udev->dev, "Result of vendor_command: %d\n\n", result);
+       if (result < 0)
+               return result;

        return sprintf(buf, "%d", cyp->port[port_num]);
 }
--
2.39.5

Re: [PATCH] usb: misc: cypress_cy7c63: check control transfer status

[Oliver Neukum]

On 10.07.26 01:30, Keshav Verma wrote:
> read_port() currently ignores errors from vendor_command() and always
> returns the cached port value. This can report stale data when the USB
> control transfer fails or returns a short response.
> 
> Convert vendor_command() to usb_control_msg_recv(), which returns 0 on
> success and a negative errno on failure, and propagate transport errors
> from read_port() instead of returning cached data.

Hi,

Thank you for making a patch for this issue.
I am sorry, but this approach mixes issues that are real with something
that is not a problem.
  
> Fixes: 9189bfc2df0f ("[PATCH] USB: rename Cypress CY7C63xxx driver to
> proper name and fix up some tiny things")
> Signed-off-by: Keshav Verma <iganschel@gmail.com>
> ---
> v2:

> -       retval = usb_control_msg(dev->udev, pipe, request,
> -                                USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_OTHER,
> -                                address, data, iobuf, CYPRESS_MAX_REQSIZE,
> -                                USB_CTRL_GET_TIMEOUT);

This does return errors.

> -       /* we must not process garbage */
> -       if (retval < 2)
> +       dev_dbg(&dev->udev->dev, "Sending usb_control_msg_recv (data:
> %d)\n", data);
> +
> +       retval = usb_control_msg_recv(dev->udev, 0, request,

There is no point in this change. You just allocate yet another
buffer for no gain.

	Regards
		Oliver

Re: [PATCH] usb: misc: cypress_cy7c63: check control transfer status

[Keshav Verma]

On Fri, Jul 10, 2026 at 3:23 PM Oliver Neukum <oneukum@suse.com> wrote:

> There is no point in this change. You just allocate yet another
> buffer for no gain.
>
>         Regards
>                 Oliver
>

Hi,

Thanks for the review.

The bug I was trying to address is that read_port() ignores the return
value from vendor_command() and can return stale cached data after a
failed control transfer.

I updated vendor_command() to use usb_control_msg_recv() based on
review feedback on the previous version.

Would you prefer that I keep the existing usb_control_msg()
implementation and resend a v3 containing only the stale data fix?

Thanks,
Keshav Verma

Re: [PATCH] usb: misc: cypress_cy7c63: check control transfer status

[Greg KH]

On Fri, Jul 10, 2026 at 11:53:31AM +0200, Oliver Neukum wrote:
> On 10.07.26 01:30, Keshav Verma wrote:
> > read_port() currently ignores errors from vendor_command() and always
> > returns the cached port value. This can report stale data when the USB
> > control transfer fails or returns a short response.
> > 
> > Convert vendor_command() to usb_control_msg_recv(), which returns 0 on
> > success and a negative errno on failure, and propagate transport errors
> > from read_port() instead of returning cached data.
> 
> Hi,
> 
> Thank you for making a patch for this issue.
> I am sorry, but this approach mixes issues that are real with something
> that is not a problem.
> > Fixes: 9189bfc2df0f ("[PATCH] USB: rename Cypress CY7C63xxx driver to
> > proper name and fix up some tiny things")
> > Signed-off-by: Keshav Verma <iganschel@gmail.com>
> > ---
> > v2:
> 
> > -       retval = usb_control_msg(dev->udev, pipe, request,
> > -                                USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_OTHER,
> > -                                address, data, iobuf, CYPRESS_MAX_REQSIZE,
> > -                                USB_CTRL_GET_TIMEOUT);
> 
> This does return errors.

Yes, but they are not checked properly, as your patch showed.

> > -       /* we must not process garbage */
> > -       if (retval < 2)
> > +       dev_dbg(&dev->udev->dev, "Sending usb_control_msg_recv (data:
> > %d)\n", data);
> > +
> > +       retval = usb_control_msg_recv(dev->udev, 0, request,
> 
> There is no point in this change. You just allocate yet another
> buffer for no gain.

That's not an issue, we should be moving all drivers away from
usb_control_msg() as it's used wrong so many times (like this one.)

So I think this is the correct thing to be doing here.

thanks,

greg k-h