Re: [PATCH 1/2] USB: serial: cp210x: Implement 16-bit register access functions

[Oliver Neukum <oneukum@suse.com>]

On Mon, 2015-10-19 at 17:01 -0500, Konstantin Shkolnyy wrote:
> Existing register access functions cp210x_get_config and cp210x_set_config
> are cumbersome to use. This change introduces new functions specifically
> for 16-bit registers that read and write simple u16 values.

Hi,

I am afraid there are some issues.

> 
> Signed-off-by: Konstantin Shkolnyy <konstantin.shkolnyy@gmail.com>
> ---
>  drivers/usb/serial/cp210x.c | 119 ++++++++++++++++++++++++++++++++------------
>  1 file changed, 88 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/usb/serial/cp210x.c b/drivers/usb/serial/cp210x.c
> index eac7cca..5a7c15e 100644
> --- a/drivers/usb/serial/cp210x.c
> +++ b/drivers/usb/serial/cp210x.c
> @@ -301,6 +301,77 @@ static struct usb_serial_driver * const serial_drivers[] = {
>  #define CONTROL_WRITE_RTS	0x0200
>  
>  /*
> + * Reads any 16-bit CP210X_ register (req).
> + */
> +static int cp210x_read_u16_reg(struct usb_serial *serial, u8 req, u16 *pval)
> +{
> +	struct cp210x_serial_private *spriv = usb_get_serial_data(serial);
> +	__le16 le16_value;

1. If you already get passed a pointer to a buffer, why use another
buffer?
2. You are doing DMA on the stack. That is forbidden.

> +	int result;
> +
> +	result = usb_control_msg(serial->dev,
> +				usb_rcvctrlpipe(serial->dev, 0),
> +				req, REQTYPE_INTERFACE_TO_HOST, 0,
> +				spriv->bInterfaceNumber, &le16_value, 2,
> +				USB_CTRL_SET_TIMEOUT);
> +	if (result != 2) {
> +		if (result > 0)
> +			result = -EPROTO;
> +		dev_err(&serial->dev->dev, "%s ifc %d req 0x%x err %d\n",
> +			__func__, spriv->bInterfaceNumber, req, result);

It would make more sense inverting the debug statement and the error
conversion, as you wouldn't lose information.

> +		return result;
> +	}
> +	*pval = le16_to_cpu(le16_value);
> +	return 0;
> +}

	Regards
		Oliver