From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752463AbbJTGrH (ORCPT ); Tue, 20 Oct 2015 02:47:07 -0400 Received: from mx2.suse.de ([195.135.220.15]:56767 "EHLO mx2.suse.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751051AbbJTGrF (ORCPT ); Tue, 20 Oct 2015 02:47:05 -0400 Message-ID: <1445323506.2246.6.camel@suse.com> Subject: Re: [PATCH 1/2] USB: serial: cp210x: Implement 16-bit register access functions From: Oliver Neukum To: Konstantin Shkolnyy Cc: johan@kernel.org, linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org Date: Tue, 20 Oct 2015 08:45:06 +0200 In-Reply-To: <1445292084-27910-1-git-send-email-konstantin.shkolnyy@gmail.com> References: <1445292084-27910-1-git-send-email-konstantin.shkolnyy@gmail.com> Content-Type: text/plain; charset="UTF-8" X-Mailer: Evolution 3.12.11 Mime-Version: 1.0 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > --- > 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