Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
Subject: Re: [PATCH v3 02/12] usb: misc: cypress_cy7c63: update to use
 usb_control_msg_recv()
To:     Johan Hovold <johan@kernel.org>
Cc:     Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org
References: <20210126183403.911653-1-anant.thazhemadam@gmail.com>
 <20210126183403.911653-3-anant.thazhemadam@gmail.com>
 <YBF0B50Q9gi/Ezpz@hovoldconsulting.com>
From:   Anant Thazhemadam <anant.thazhemadam@gmail.com>
Message-ID: <2347687a-d92e-d06e-a8b5-a67a45d5040b@gmail.com>
Date:   Wed, 27 Jan 2021 20:10:45 +0530
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.10.0
MIME-Version: 1.0
In-Reply-To: <YBF0B50Q9gi/Ezpz@hovoldconsulting.com>
Content-Type: text/plain; charset=windows-1252
Content-Transfer-Encoding: 7bit
Content-Language: en-US
Precedence: bulk


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@gmail.com>
>> ---
>>  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