| Message ID | 20201129160612.1908074-4-anant.thazhemadam@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | drivers: usb: misc: update to use usb_control_msg_{send|recv}() | expand |
On Sun, Nov 29, 2020 at 09:36:00PM +0530, Anant Thazhemadam wrote: > The newer usb_control_msg_{send|recv}() API are an improvement on the > existing usb_control_msg() as it ensures that a short read/write is treated > as an error, data can be used off the stack, and raw usb pipes need not be > created in the calling functions. > For this reason, the instance of usb_control_msg() has been replaced with > usb_control_msg_recv(). > > The return value checking enforced by callers of the updated function have > also been appropriately updated. > > Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com> > --- > drivers/usb/misc/cytherm.c | 42 +++++++++++++++++++------------------- > 1 file changed, 21 insertions(+), 21 deletions(-) > > diff --git a/drivers/usb/misc/cytherm.c b/drivers/usb/misc/cytherm.c > index 3e3802aaefa3..75fa4d806a77 100644 > --- a/drivers/usb/misc/cytherm.c > +++ b/drivers/usb/misc/cytherm.c > @@ -51,12 +51,12 @@ 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); > } > > > @@ -95,14 +95,14 @@ static ssize_t brightness_store(struct device *dev, struct device_attribute *att > /* Set brightness */ > retval = vendor_command(cytherm->udev, WRITE_RAM, BRIGHTNESS, > cytherm->brightness, buffer, 8); > - if (retval) > - dev_dbg(&cytherm->udev->dev, "retval = %d\n", retval); > + if (!retval) > + dev_dbg(&cytherm->udev->dev, "brightness set correctly\n"); > /* Inform µC that we have changed the brightness setting */ > retval = vendor_command(cytherm->udev, WRITE_RAM, BRIGHTNESS_SEM, > 0x01, buffer, 8); > - if (retval) > - dev_dbg(&cytherm->udev->dev, "retval = %d\n", retval); > - > + if (!retval) > + dev_dbg(&cytherm->udev->dev, "µC informed of change in brightness setting\n"); > + > kfree(buffer); > > return count; There's no more need to dynamically allocate the buffer variable here now, right? It can be on the stack as the change you made above allows that to work properly, no need to allocate the buffer twice in a row (once here and once in the USB core). That would make these functions less complex, always a good thing. You should check this on the other patches in this series as well. thanks, greg k-h
On 29/11/20 9:46 pm, Greg Kroah-Hartman wrote: > There's no more need to dynamically allocate the buffer variable here > now, right? It can be on the stack as the change you made above allows > that to work properly, no need to allocate the buffer twice in a row > (once here and once in the USB core). > > That would make these functions less complex, always a good thing. > > You should check this on the other patches in this series as well. Understood, I will do that. I apologize for the oversight. Thanks, Anant
diff --git a/drivers/usb/misc/cytherm.c b/drivers/usb/misc/cytherm.c index 3e3802aaefa3..75fa4d806a77 100644 --- a/drivers/usb/misc/cytherm.c +++ b/drivers/usb/misc/cytherm.c @@ -51,12 +51,12 @@ 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); } @@ -95,14 +95,14 @@ static ssize_t brightness_store(struct device *dev, struct device_attribute *att /* Set brightness */ retval = vendor_command(cytherm->udev, WRITE_RAM, BRIGHTNESS, cytherm->brightness, buffer, 8); - if (retval) - dev_dbg(&cytherm->udev->dev, "retval = %d\n", retval); + if (!retval) + dev_dbg(&cytherm->udev->dev, "brightness set correctly\n"); /* Inform µC that we have changed the brightness setting */ retval = vendor_command(cytherm->udev, WRITE_RAM, BRIGHTNESS_SEM, 0x01, buffer, 8); - if (retval) - dev_dbg(&cytherm->udev->dev, "retval = %d\n", retval); - + if (!retval) + dev_dbg(&cytherm->udev->dev, "µC informed of change in brightness setting\n"); + kfree(buffer); return count; @@ -130,14 +130,14 @@ static ssize_t temp_show(struct device *dev, struct device_attribute *attr, char /* read temperature */ retval = vendor_command(cytherm->udev, READ_RAM, TEMP, 0, buffer, 8); - if (retval) - dev_dbg(&cytherm->udev->dev, "retval = %d\n", retval); + if (!retval) + dev_dbg(&cytherm->udev->dev, "read temperature successfully\n"); temp = buffer[1]; /* read sign */ retval = vendor_command(cytherm->udev, READ_RAM, SIGN, 0, buffer, 8); - if (retval) - dev_dbg(&cytherm->udev->dev, "retval = %d\n", retval); + if (!retval) + dev_dbg(&cytherm->udev->dev, "read sign successfully\n"); sign = buffer[1]; kfree(buffer); @@ -165,8 +165,8 @@ static ssize_t button_show(struct device *dev, struct device_attribute *attr, ch /* check button */ retval = vendor_command(cytherm->udev, READ_RAM, BUTTON, 0, buffer, 8); - if (retval) - dev_dbg(&cytherm->udev->dev, "retval = %d\n", retval); + if (!retval) + dev_dbg(&cytherm->udev->dev, "checked button successfully\n"); retval = buffer[1]; @@ -193,7 +193,7 @@ static ssize_t port0_show(struct device *dev, struct device_attribute *attr, cha return 0; retval = vendor_command(cytherm->udev, READ_PORT, 0, 0, buffer, 8); - if (retval) + if (!retval) dev_dbg(&cytherm->udev->dev, "retval = %d\n", retval); retval = buffer[1]; @@ -226,7 +226,7 @@ static ssize_t port0_store(struct device *dev, struct device_attribute *attr, co retval = vendor_command(cytherm->udev, WRITE_PORT, 0, tmp, buffer, 8); - if (retval) + if (!retval) dev_dbg(&cytherm->udev->dev, "retval = %d\n", retval); kfree(buffer); @@ -248,7 +248,7 @@ static ssize_t port1_show(struct device *dev, struct device_attribute *attr, cha return 0; retval = vendor_command(cytherm->udev, READ_PORT, 1, 0, buffer, 8); - if (retval) + if (!retval) dev_dbg(&cytherm->udev->dev, "retval = %d\n", retval); retval = buffer[1]; @@ -281,7 +281,7 @@ static ssize_t port1_store(struct device *dev, struct device_attribute *attr, co retval = vendor_command(cytherm->udev, WRITE_PORT, 1, tmp, buffer, 8); - if (retval) + if (!retval) dev_dbg(&cytherm->udev->dev, "retval = %d\n", retval); kfree(buffer);
The newer usb_control_msg_{send|recv}() API are an improvement on the existing usb_control_msg() as it ensures that a short read/write is treated as an error, data can be used off the stack, and raw usb pipes need not be created in the calling functions. For this reason, the instance of usb_control_msg() has been replaced with usb_control_msg_recv(). The return value checking enforced by callers of the updated function have also been appropriately updated. Signed-off-by: Anant Thazhemadam <anant.thazhemadam@gmail.com> --- drivers/usb/misc/cytherm.c | 42 +++++++++++++++++++------------------- 1 file changed, 21 insertions(+), 21 deletions(-)