usb: misc: cypress_cy7c63: check control transfer status

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.

---
 drivers/usb/misc/cypress_cy7c63.c | 4 ++++
 1 file changed, 4 insertions(+)

index 4a7f955ba85b..8601ee0a1ea3 100644
@@ -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

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

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.

proper name and fix up some tiny things")

---
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(-)

b/drivers/usb/misc/cypress_cy7c63.c
index 4a7f955ba85b..47cb05c3a870 100644

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

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.

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

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

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