Re: [PATCH] Driver for MaxLinear/Exar USB (UART) Serial Adapters
On Thu, Aug 16, 2018 at 01:28:54AM -0700, Patong Yang wrote: > On Thu, Aug 16, 2018 at 08:34:47AM +0200, Greg KH wrote: > > On Wed, Aug 15, 2018 at 10:56:47PM -0700, Patong Yang wrote: > > > Greg, > > > > > > Please see my response inline below. > > > > > > Patong > > > > > > > But there is a bigger problem here: > > > > > > > > > + xrusb_tty_driver = alloc_tty_driver(XRUSB_TTY_MINORS); > > > > > + if (!xrusb_tty_driver) > > > > > + return -ENOMEM; > > > > > > > > Why are you not using the usb serial core here? You need to do that, > > > > not try to provide your own custom tty driver. That way userspace > > > > programs will "just work" with your new device, no changes needed as > > > > your major/minor number and device name would be custom only for your > > > > device, which is not acceptable. > > > > > > The MaxLinear/USB serial devices support the CDC-ACM commands. > > > Therefore, we used the cdc-acm driver instead of the usb serial driver > > > as the starting point for developing the driver. We replaced "ACM" > > > with "XRUSB" throughout the driver. Would it be better if we just used > > > the same major/minor number as the CDC-ACM driver since it was based on > > > the cdc-acm driver? > > > > No, just use the cdc-acm driver itself and add your product/device id to > > it and it should work just fine. Why do you need to write a whole new > > driver at all? > > The basic TX/RX functionality works fine now with the cdc-acm driver. > That's the driver that is loaded because it's advertised as a cdc-acm > compatible device in the device descriptors. > > However, the cdc-acm driver (and spec) does not have support all of the > features in the MaxLinear/Exar USB UARTs. Hence the reason for a separate > and new driver. So your device should not be exposing itself as a cdc-acm driver if it is doing vendor-specific things, right? Ok, that's fine, but then you still need to tie into the usb-serial core, just use that and do not implement all of the duplicated tty handling logic that you have done here. You will end up with a ttyUSB* device node, which is what you, and your users want, not some custom-name that no program supports. thanks, greg k-h
Re: [PATCH] Driver for MaxLinear/Exar USB (UART) Serial Adapters
On Do, 2018-08-16 at 01:28 -0700, Patong Yang wrote: > Features not supported by the cdc-acm driver are > enabling/disabling flow control, enabling/disabling RS-485 mode, > GPIOs and GPIO modes, etc. Support for these features had to be added in this > new driver. Hi, I am afraid RS-485 in cdc-acm is a no-go. I don't like the duplication, but it is the lesser evil. We might want to include more stuff in the new driver, so we duplicate only compiled code. Regards Oliver
Re: [PATCH] Driver for MaxLinear/Exar USB (UART) Serial Adapters
On Thu, Aug 16, 2018 at 08:34:47AM +0200, Greg KH wrote: > On Wed, Aug 15, 2018 at 10:56:47PM -0700, Patong Yang wrote: > > Greg, > > > > Please see my response inline below. > > > > Patong > > > > > But there is a bigger problem here: > > > > > > > + xrusb_tty_driver = alloc_tty_driver(XRUSB_TTY_MINORS); > > > > + if (!xrusb_tty_driver) > > > > + return -ENOMEM; > > > > > > Why are you not using the usb serial core here? You need to do that, > > > not try to provide your own custom tty driver. That way userspace > > > programs will "just work" with your new device, no changes needed as > > > your major/minor number and device name would be custom only for your > > > device, which is not acceptable. > > > > The MaxLinear/USB serial devices support the CDC-ACM commands. > > Therefore, we used the cdc-acm driver instead of the usb serial driver > > as the starting point for developing the driver. We replaced "ACM" > > with "XRUSB" throughout the driver. Would it be better if we just used > > the same major/minor number as the CDC-ACM driver since it was based on > > the cdc-acm driver? > > No, just use the cdc-acm driver itself and add your product/device id to > it and it should work just fine. Why do you need to write a whole new > driver at all? The basic TX/RX functionality works fine now with the cdc-acm driver. That's the driver that is loaded because it's advertised as a cdc-acm compatible device in the device descriptors. However, the cdc-acm driver (and spec) does not have support all of the features in the MaxLinear/Exar USB UARTs. Hence the reason for a separate and new driver. Features not supported by the cdc-acm driver are enabling/disabling flow control, enabling/disabling RS-485 mode, GPIOs and GPIO modes, etc. Support for these features had to be added in this new driver. Please let me know if there are any futher questions and advise on how to proceed. Thanks, Patong
Re: [PATCH 1/2] USB: yurex: Fix buffer over-read in yurex_write()
On Thu, 2018-08-16 at 04:15 +0200, Jann Horn wrote:
> On Wed, Aug 15, 2018 at 10:44 PM Ben Hutchings
> wrote:
[...]
> > @@ -446,6 +446,7 @@ static ssize_t yurex_write(struct file *file, const
> > char __user *user_buffer,
> > retval = -EFAULT;
> > goto error;
> > }
> > + buffer[count] = 0;
> > memset(dev->cntl_buffer, CMD_PADDING, YUREX_BUF_SIZE);
> >
> > switch (buffer[0]) {
>
> By the way: A little bit below here, there's some other line that looks bogus:
>
> > buffer[6] = CMD_EOF;
>
> AFAICS that should probably go into ->cntl_buffer; `buffer` and `data`
> aren't used below that point. But that'd just be a functional bug, not
> security-relevant, so I'm not sure whether anyone cares.
Yes I noticed that too. Since I can't test with the actual hardware, I
left it alone. For all I know, the firmware actually expects
CMD_PADDING and not CMD_EOF at the end of this command.
Ben.
--
Ben Hutchings, Software Developer Codethink Ltd
https://www.codethink.co.uk/ Dale House, 35 Dale Street
Manchester, M1 2HF, United Kingdom
