On 27/01/21 7:39 pm, Johan Hovold wrote:
> On Wed, Jan 27, 2021 at 12:03:53AM +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
> Short write has always been an error (I won't repeat for the remaining
> patches).
>
>> 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().
>>
>> Signed-off-by: Anant Thazhemadam <anant.thazhemadam@xxxxxxxxx>
>> ---
>> drivers/usb/misc/cypress_cy7c63.c | 21 +++++----------------
>> 1 file changed, 5 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/usb/misc/cypress_cy7c63.c b/drivers/usb/misc/cypress_cy7c63.c
>> index 14faec51d7a5..76a320ef17a7 100644
>> --- a/drivers/usb/misc/cypress_cy7c63.c
>> +++ b/drivers/usb/misc/cypress_cy7c63.c
>> @@ -70,24 +70,15 @@ static int vendor_command(struct cypress *dev, unsigned char request,
>> unsigned char address, unsigned char data)
>> {
>> int retval = 0;
>> - unsigned int pipe;
>> - unsigned char *iobuf;
>> -
>> - /* allocate some memory for the i/o buffer*/
>> - iobuf = kzalloc(CYPRESS_MAX_REQSIZE, GFP_KERNEL);
>> - if (!iobuf) {
>> - retval = -ENOMEM;
>> - goto error;
>> - }
>> + u8 iobuf[CYPRESS_MAX_REQSIZE] = {0};
>>
>> 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);
>> + 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);
> Are you sure that the device always returns CYPRESS_MAX_REQSIZE here?
> Otherwise, this change may break the driver as it currently only uses
> the first two bytes of the received message, and only for some requests.
>
> Note that the driver appears uses the same helper function for
> CYPRESS_WRITE_PORT commands, which probably doesn't return 8 bytes in a
> reply.
>
> You could possibly add the missing short read check for the
> CYPRESS_READ_PORT case, but I'm afraid that the new helper are not a
> good fit here either.
>
Understood, but I think that change might be better proposed (for this, and cytherm, both)
separately from this series, so I'll just drop it from this series for now.
Thanks,
Anant
