* [PATCH v9 2/5] serdev: Introduce devm_serdev_device_open()
From: Andrey Smirnov @ 2017-10-25 19:04 UTC (permalink / raw)
To: linux-kernel
Cc: Andrey Smirnov, linux-serial, Rob Herring, cphealy, Guenter Roeck,
Lucas Stach, Nikita Yushchenko, Lee Jones, Greg Kroah-Hartman,
Pavel Machek, Andy Shevchenko, Johan Hovold
In-Reply-To: <20171025190421.18415-1-andrew.smirnov@gmail.com>
Add code implementing managed version of serdev_device_open() for
serdev device drivers that "open" the device during driver's lifecycle
only once (e.g. opened in .probe() and closed in .remove()).
Cc: linux-kernel@vger.kernel.org
Cc: linux-serial@vger.kernel.org
Cc: Rob Herring <robh@kernel.org>
Cc: cphealy@gmail.com
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Johan Hovold <johan@kernel.org>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
Documentation/driver-model/devres.txt | 3 +++
drivers/tty/serdev/core.c | 27 +++++++++++++++++++++++++++
include/linux/serdev.h | 1 +
3 files changed, 31 insertions(+)
diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
index 69f08c0f23a8..e9c6b5cfeec1 100644
--- a/Documentation/driver-model/devres.txt
+++ b/Documentation/driver-model/devres.txt
@@ -383,6 +383,9 @@ RESET
devm_reset_control_get()
devm_reset_controller_register()
+SERDEV
+ devm_serdev_device_open()
+
SLAVE DMA ENGINE
devm_acpi_dma_controller_register()
diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index f500f6a2ca88..b3a785665c6f 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -116,6 +116,33 @@ void serdev_device_close(struct serdev_device *serdev)
}
EXPORT_SYMBOL_GPL(serdev_device_close);
+static void devm_serdev_device_release(struct device *dev, void *dr)
+{
+ serdev_device_close(*(struct serdev_device **)dr);
+}
+
+int devm_serdev_device_open(struct device *dev, struct serdev_device *serdev)
+{
+ struct serdev_device **dr;
+ int ret;
+
+ dr = devres_alloc(devm_serdev_device_release, sizeof(*dr), GFP_KERNEL);
+ if (!dr)
+ return -ENOMEM;
+
+ ret = serdev_device_open(serdev);
+ if (ret) {
+ devres_free(dr);
+ return ret;
+ }
+
+ *dr = serdev;
+ devres_add(dev, dr);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(devm_serdev_device_open);
+
void serdev_device_write_wakeup(struct serdev_device *serdev)
{
complete(&serdev->write_comp);
diff --git a/include/linux/serdev.h b/include/linux/serdev.h
index e69402d4a8ae..9929063bd45d 100644
--- a/include/linux/serdev.h
+++ b/include/linux/serdev.h
@@ -193,6 +193,7 @@ static inline int serdev_controller_receive_buf(struct serdev_controller *ctrl,
int serdev_device_open(struct serdev_device *);
void serdev_device_close(struct serdev_device *);
+int devm_serdev_device_open(struct device *, struct serdev_device *);
unsigned int serdev_device_set_baudrate(struct serdev_device *, unsigned int);
void serdev_device_set_flow_control(struct serdev_device *, bool);
int serdev_device_write_buf(struct serdev_device *, const unsigned char *, size_t);
--
2.13.5
^ permalink raw reply related
* [PATCH v9 1/5] serdev: Make .remove in struct serdev_device_driver optional
From: Andrey Smirnov @ 2017-10-25 19:04 UTC (permalink / raw)
To: linux-kernel
Cc: Andrey Smirnov, linux-serial, Rob Herring, cphealy, Guenter Roeck,
Lucas Stach, Nikita Yushchenko, Lee Jones, Greg Kroah-Hartman,
Pavel Machek, Andy Shevchenko, Johan Hovold
In-Reply-To: <20171025190421.18415-1-andrew.smirnov@gmail.com>
Using devres infrastructure it is possible to write a serdev driver
that doesn't have any code that needs to be called as a part of
.remove. Add code to make .remove optional.
Cc: linux-kernel@vger.kernel.org
Cc: linux-serial@vger.kernel.org
Cc: Rob Herring <robh@kernel.org>
Cc: cphealy@gmail.com
Cc: Guenter Roeck <linux@roeck-us.net>
Cc: Lucas Stach <l.stach@pengutronix.de>
Cc: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
Cc: Lee Jones <lee.jones@linaro.org>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Pavel Machek <pavel@ucw.cz>
Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
Cc: Johan Hovold <johan@kernel.org>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
---
drivers/tty/serdev/core.c | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
index c68fb3a8ea1c..f500f6a2ca88 100644
--- a/drivers/tty/serdev/core.c
+++ b/drivers/tty/serdev/core.c
@@ -252,8 +252,8 @@ static int serdev_drv_probe(struct device *dev)
static int serdev_drv_remove(struct device *dev)
{
const struct serdev_device_driver *sdrv = to_serdev_device_driver(dev->driver);
-
- sdrv->remove(to_serdev_device(dev));
+ if (sdrv->remove)
+ sdrv->remove(to_serdev_device(dev));
return 0;
}
--
2.13.5
^ permalink raw reply related
* RE: [patch v9 1/4] drivers: jtag: Add JTAG core driver
From: Oleksandr Shamray @ 2017-10-25 15:58 UTC (permalink / raw)
To: Greg KH
Cc: arnd-r2nGTMty4D4@public.gmane.org,
linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
linux-arm-kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org,
devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
openbmc-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org,
joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org,
jiri-rHqAuBHg3fBzbRFIqnYvSA@public.gmane.org,
tklauser-93Khv+1bN0NyDzI6CaY1VQ@public.gmane.org,
linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org,
mec-WqBc5aa1uDFeoWH0uzbU5w@public.gmane.org, Vadim Pasternak,
system-sw-low-level,
robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org,
openocd-devel-owner-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org
In-Reply-To: <20171020145417.GD8965-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>
> -----Original Message-----
> From: Greg KH [mailto:gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org]
> Sent: Friday, October 20, 2017 5:54 PM
> To: Oleksandr Shamray <oleksandrs-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Cc: arnd-r2nGTMty4D4@public.gmane.org; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-arm-
> kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> openbmc-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org; joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org; jiri-rHqAuBHg3fBzbRFIqnYvSA@public.gmane.org;
> tklauser-93Khv+1bN0NyDzI6CaY1VQ@public.gmane.org; linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; mec-WqBc5aa1uDFeoWH0uzbU5w@public.gmane.org; Vadim
> Pasternak <vadimp-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; system-sw-low-level <system-sw-low-
> level-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; openocd-devel-
> owner-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org; linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org; mchehab-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org; Jiri Pirko <jiri-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> Subject: Re: [patch v9 1/4] drivers: jtag: Add JTAG core driver
>
> On Fri, Oct 20, 2017 at 02:34:00PM +0000, Oleksandr Shamray wrote:
> > Hi Greg.
> >
> > > -----Original Message-----
> > > From: Greg KH [mailto:gregkh-hQyY1W1yCW8ekmWlsbkhG0B+6BGkLq7r@public.gmane.org]
> > > Sent: Friday, October 20, 2017 2:55 PM
> > > To: Oleksandr Shamray <oleksandrs-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > > Cc: arnd-r2nGTMty4D4@public.gmane.org; linux-kernel-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; linux-arm-
> > > kernel-IAPFreCvJWM7uuMidbF8XUB+6BGkLq7r@public.gmane.org; devicetree-u79uwXL29TY76Z2rM5mHXA@public.gmane.org;
> > > openbmc-uLR06cmDAlY/bJ5BZ2RsiQ@public.gmane.org; joel-U3u1mxZcP9KHXe+LvDLADg@public.gmane.org; jiri-rHqAuBHg3fBzbRFIqnYvSA@public.gmane.org;
> > > tklauser-93Khv+1bN0NyDzI6CaY1VQ@public.gmane.org; linux-serial-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; mec-WqBc5aa1uDFeoWH0uzbU5w@public.gmane.org;
> > > Vadim Pasternak <vadimp-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; system-sw-low-level
> > > <system-sw-low- level-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>; robh+dt-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org;
> > > openocd-devel- owner-5NWGOfrQmneRv+LV9MX5uipxlwaOVQ5f@public.gmane.org;
> > > linux-api-u79uwXL29TY76Z2rM5mHXA@public.gmane.org; davem-fT/PcQaiUtIeIZ0/mPfg9Q@public.gmane.org; mchehab-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org;
> > > Jiri Pirko <jiri-VPRAkNaXOzVWk0Htik3J/w@public.gmane.org>
> > > Subject: Re: [patch v9 1/4] drivers: jtag: Add JTAG core driver
> > >
> > > On Thu, Sep 21, 2017 at 12:25:29PM +0300, Oleksandr Shamray wrote:
> > > > +struct jtag {
> > > > + struct device *dev;
> > > > + struct cdev cdev;
> > >
> > > Why are you using a cdev here and not just a normal misc device?
> >
> > What the benefits to use misc instead of cdev?
>
> Less code, simpler logic, easier to review and understand, etc.
>
> Let me ask you, why use a cdev instead of a misc?
As I know misc device more applicable if we want to create one device f.e. /dev/jtag.
But in current case we can have more than one jtag device /dev/jtag0 ... /dev/jtagN.
So I decided to use cdev.
>
> > > I forgot if this is what you were doing before, sorry...
> > >
> > > > + int id;
> > > > + atomic_t open;
> > >
> > > Why do you need this?
> >
> > This counter used to avoid open at the same time by 2 or more users.
>
> But it isn't working :)
>
> And why do you care?
>
> > > > + const struct jtag_ops *ops;
> > > > + unsigned long priv[0] __aligned(ARCH_DMA_MINALIGN);
> > >
> > > Huh? Why is this needed to be dma aligned? Why not just use the
> > > private pointer in struct device?
> > >
> >
> > It is critical?
>
> You are saying it is, so you have to justify it. There is a pointer for you to use,
> don't make new ones for no reason, right?
>
You are right. Will remove.
> > > > +};
> > > > +
> > > > +static dev_t jtag_devt;
> > > > +static DEFINE_IDA(jtag_ida);
> > > > +
> > > > +void *jtag_priv(struct jtag *jtag) {
> > > > + return jtag->priv;
> > > > +}
> > > > +EXPORT_SYMBOL_GPL(jtag_priv);
> > > > +
> > > > +static u8 *jtag_copy_from_user(__u64 udata, unsigned long bit_size) {
> > > > + unsigned long size;
> > > > + void *kdata;
> > > > +
> > > > + size = DIV_ROUND_UP(bit_size, BITS_PER_BYTE);
> > > > + kdata = memdup_user(u64_to_user_ptr(udata), size);
> > >
> > > You only use this once, why not just open-code it?
> >
> > I think it makes code more understandable.
>
> As a reviewer, I don't :)
Ok, I will fix :)
>
> > > > +
> > > > + return kdata;
> > > > +}
> > > > +
> > > > +static unsigned long jtag_copy_to_user(__u64 udata, u8 *kdata,
> > > > + unsigned long bit_size) {
> > > > + unsigned long size;
> > > > +
> > > > + size = DIV_ROUND_UP(bit_size, BITS_PER_BYTE);
> > > > +
> > > > + return copy_to_user(u64_to_user_ptr(udata), (void *)(kdata),
> > > > +size);
> > >
> > > Same here, making this a separate function seems odd.
> >
> > Same, I think it makes code more understandable.
>
> But it doesn't.
>
Ok, I will fix :)
> > > > +
> > > > + if (jtag->ops->freq_set)
> > > > + err = jtag->ops->freq_set(jtag, value);
> > > > + else
> > > > + err = -EOPNOTSUPP;
> > > > + break;
> > > > +
> > > > + case JTAG_IOCRUNTEST:
> > > > + if (copy_from_user(&idle, (void *)arg,
> > > > + sizeof(struct jtag_run_test_idle)))
> > > > + return -ENOMEM;
> > > > + err = jtag_run_test_idle_op(jtag, &idle);
> > >
> > > Who validates the structure fields? Is that up to the individual
> > > jtag driver? Why not do it in the core correctly so that it only
> > > has to be done in one place and you do not have to audit every individual
> driver?
> >
> > Input parameters validated by jtag platform driver. I think it not critical.
>
> Not true at all. It is very critical. Remmeber, "All Input Is Evil!"
>
> You have to validate this. I as a reviewer have to find where you are validating
> this data to ensure bad things do not happen. I can't review that here, now I
> have to go and review all of the individual drivers, which is a major pain, don't
> you agree?
Agree. I will add input parameter checking here before call device driver.
>
> > > > + break;
> > > > +
> > > > + case JTAG_IOCXFER:
> > > > + if (copy_from_user(&xfer, (void *)arg,
> > > > + sizeof(struct jtag_xfer)))
> > > > + return -EFAULT;
> > > > +
> > > > + if (xfer.length >= JTAG_MAX_XFER_DATA_LEN)
> > > > + return -EFAULT;
> > > > +
> > > > + xfer_data = jtag_copy_from_user(xfer.tdio, xfer.length);
> > > > + if (!xfer_data)
> > > > + return -ENOMEM;
> > >
> > > Are you sure that's the correct error value?
> >
> > I think yes, but what you suggest?
>
> A fault happened, so -EFAULT, right?
>
Right.
> > [..]
> > > + .unlocked_ioctl = jtag_ioctl,
> > > + .open = jtag_open,
> > > + .release = jtag_release,
> > > +};
> >
> > add a compat_ioctl pointer here, after ensuring that all ioctl
> > commands are compatible between 32-bit and 64-bit user space.
> > [..]
>
> And if you do not, what happens? You shouldn't need it as there is no fixups
> necessary, or am I mistaken about that?
Yes, you are right. In code compat_ioctl called same function as in unlocked_ioctl.
So I can remove compat and system will always call unlocked_ioctl.
>
> > > > +static int jtag_open(struct inode *inode, struct file *file) {
> > > > + struct jtag *jtag = container_of(inode->i_cdev, struct jtag,
> > > > +cdev);
> > > > +
> > > > + if (atomic_read(&jtag->open)) {
> > > > + dev_info(NULL, "jtag already opened\n");
> > > > + return -EBUSY;
> > >
> > > Why do you care if multiple opens can happen?
> >
> > Jtag HW not support to using with multiple requests from different users. So
> we prohibit this.
>
> Why does the kernel care?
>
> And again, your implementation is broken, it's not actually doing this
> protection. I recommend just not doing it at all, but if you really are insisting
> on it, you have to get it correct :)
I will follow your recommendations and remove it.
>
> thanks,
>
> greg k-h
Thanks.
Oleksandr S
^ permalink raw reply
* [PATCH] tty/serial: atmel: Convert timers to use timer_setup()
From: Richard Genoud @ 2017-10-24 10:54 UTC (permalink / raw)
To: Kees Cook, Greg Kroah-Hartman; +Cc: Jiri Slaby, linux-serial, linux-kernel
In-Reply-To: <20171024100003.GA67892@beast>
Le mardi 24 octobre 2017 à 03:00 -0700, Kees Cook a écrit :
> In preparation for unconditionally passing the struct timer_list
> pointer to
> all timer callbacks, switch to using the new timer_setup() and
> from_timer()
> to pass the timer pointer explicitly.
>
> Cc: Richard Genoud <richard.genoud@gmail.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jslaby@suse.com>
> Cc: linux-serial@vger.kernel.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
Acked-by: Richard Genoud <richard.genoud@gmail.com>
Thanks !
> ---
> drivers/tty/serial/atmel_serial.c | 11 +++++------
> 1 file changed, 5 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/tty/serial/atmel_serial.c
> b/drivers/tty/serial/atmel_serial.c
> index 82d9c8eae04f..68d8685e5a50 100644
> --- a/drivers/tty/serial/atmel_serial.c
> +++ b/drivers/tty/serial/atmel_serial.c
> @@ -1185,10 +1185,11 @@ static int atmel_prepare_rx_dma(struct
> uart_port *port)
> return -EINVAL;
> }
>
> -static void atmel_uart_timer_callback(unsigned long data)
> +static void atmel_uart_timer_callback(struct timer_list *t)
> {
> - struct uart_port *port = (void *)data;
> - struct atmel_uart_port *atmel_port =
> to_atmel_uart_port(port);
> + struct atmel_uart_port *atmel_port = from_timer(atmel_port,
> t,
> + uart_timer);
> + struct uart_port *port = &atmel_port->uart;
>
> if (!atomic_read(&atmel_port->tasklet_shutdown)) {
> tasklet_schedule(&atmel_port->tasklet_rx);
> @@ -1852,9 +1853,7 @@ static int atmel_startup(struct uart_port
> *port)
> atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN |
> ATMEL_US_RXEN);
> atmel_port->tx_stopped = false;
>
> - setup_timer(&atmel_port->uart_timer,
> - atmel_uart_timer_callback,
> - (unsigned long)port);
> + timer_setup(&atmel_port->uart_timer,
> atmel_uart_timer_callback, 0);
>
> if (atmel_use_pdc_rx(port)) {
> /* set UART timeout */
> --
> 2.7.4
>
>
^ permalink raw reply
* Re: [PATCH] tty/serial: altera_uart: Convert timers to use timer_setup()
From: Tobias Klauser @ 2017-10-24 10:09 UTC (permalink / raw)
To: Kees Cook
Cc: Greg Kroah-Hartman, Jiri Slaby, linux-serial, nios2-dev,
linux-kernel
In-Reply-To: <20171024095956.GA67872@beast>
On 2017-10-24 at 11:59:56 +0200, Kees Cook <keescook@chromium.org> wrote:
> In preparation for unconditionally passing the struct timer_list pointer to
> all timer callbacks, switch to using the new timer_setup() and from_timer()
> to pass the timer pointer explicitly.
>
> Cc: Tobias Klauser <tklauser@distanz.ch>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Jiri Slaby <jslaby@suse.com>
> Cc: linux-serial@vger.kernel.org
> Cc: nios2-dev@lists.rocketboards.org
> Signed-off-by: Kees Cook <keescook@chromium.org>
Acked-by: Tobias Klauser <tklauser@distanz.ch>
Thank you
^ permalink raw reply
* [PATCH] tty/serial: atmel: Convert timers to use timer_setup()
From: Kees Cook @ 2017-10-24 10:00 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Richard Genoud, Jiri Slaby, linux-serial, linux-kernel
In preparation for unconditionally passing the struct timer_list pointer to
all timer callbacks, switch to using the new timer_setup() and from_timer()
to pass the timer pointer explicitly.
Cc: Richard Genoud <richard.genoud@gmail.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: linux-serial@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/tty/serial/atmel_serial.c | 11 +++++------
1 file changed, 5 insertions(+), 6 deletions(-)
diff --git a/drivers/tty/serial/atmel_serial.c b/drivers/tty/serial/atmel_serial.c
index 82d9c8eae04f..68d8685e5a50 100644
--- a/drivers/tty/serial/atmel_serial.c
+++ b/drivers/tty/serial/atmel_serial.c
@@ -1185,10 +1185,11 @@ static int atmel_prepare_rx_dma(struct uart_port *port)
return -EINVAL;
}
-static void atmel_uart_timer_callback(unsigned long data)
+static void atmel_uart_timer_callback(struct timer_list *t)
{
- struct uart_port *port = (void *)data;
- struct atmel_uart_port *atmel_port = to_atmel_uart_port(port);
+ struct atmel_uart_port *atmel_port = from_timer(atmel_port, t,
+ uart_timer);
+ struct uart_port *port = &atmel_port->uart;
if (!atomic_read(&atmel_port->tasklet_shutdown)) {
tasklet_schedule(&atmel_port->tasklet_rx);
@@ -1852,9 +1853,7 @@ static int atmel_startup(struct uart_port *port)
atmel_uart_writel(port, ATMEL_US_CR, ATMEL_US_TXEN | ATMEL_US_RXEN);
atmel_port->tx_stopped = false;
- setup_timer(&atmel_port->uart_timer,
- atmel_uart_timer_callback,
- (unsigned long)port);
+ timer_setup(&atmel_port->uart_timer, atmel_uart_timer_callback, 0);
if (atmel_use_pdc_rx(port)) {
/* set UART timeout */
--
2.7.4
--
Kees Cook
Pixel Security
^ permalink raw reply related
* [PATCH] tty/serial: altera_uart: Convert timers to use timer_setup()
From: Kees Cook @ 2017-10-24 9:59 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Tobias Klauser, Jiri Slaby, linux-serial, nios2-dev, linux-kernel
In preparation for unconditionally passing the struct timer_list pointer to
all timer callbacks, switch to using the new timer_setup() and from_timer()
to pass the timer pointer explicitly.
Cc: Tobias Klauser <tklauser@distanz.ch>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: linux-serial@vger.kernel.org
Cc: nios2-dev@lists.rocketboards.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/tty/serial/altera_uart.c | 8 ++++----
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/altera_uart.c b/drivers/tty/serial/altera_uart.c
index 3e4b717670d7..a0360e640837 100644
--- a/drivers/tty/serial/altera_uart.c
+++ b/drivers/tty/serial/altera_uart.c
@@ -288,10 +288,10 @@ static irqreturn_t altera_uart_interrupt(int irq, void *data)
return IRQ_RETVAL(isr);
}
-static void altera_uart_timer(unsigned long data)
+static void altera_uart_timer(struct timer_list *t)
{
- struct uart_port *port = (void *)data;
- struct altera_uart *pp = container_of(port, struct altera_uart, port);
+ struct altera_uart *pp = from_timer(pp, t, tmr);
+ struct uart_port *port = &pp->port;
altera_uart_interrupt(0, port);
mod_timer(&pp->tmr, jiffies + uart_poll_timeout(port));
@@ -314,7 +314,7 @@ static int altera_uart_startup(struct uart_port *port)
int ret;
if (!port->irq) {
- setup_timer(&pp->tmr, altera_uart_timer, (unsigned long)port);
+ timer_setup(&pp->tmr, altera_uart_timer, 0);
mod_timer(&pp->tmr, jiffies + uart_poll_timeout(port));
return 0;
}
--
2.7.4
--
Kees Cook
Pixel Security
^ permalink raw reply related
* [PATCH] serial: sccnxp: Convert timers to use timer_setup()
From: Kees Cook @ 2017-10-24 9:59 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Jiri Slaby, linux-serial, linux-kernel
In preparation for unconditionally passing the struct timer_list pointer to
all timer callbacks, switch to using the new timer_setup() and from_timer()
to pass the timer pointer explicitly.
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: linux-serial@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/tty/serial/sccnxp.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/sccnxp.c b/drivers/tty/serial/sccnxp.c
index b9c7a904c1ea..ba43ed159b6a 100644
--- a/drivers/tty/serial/sccnxp.c
+++ b/drivers/tty/serial/sccnxp.c
@@ -465,9 +465,9 @@ static void sccnxp_handle_events(struct sccnxp_port *s)
} while (1);
}
-static void sccnxp_timer(unsigned long data)
+static void sccnxp_timer(struct timer_list *t)
{
- struct sccnxp_port *s = (struct sccnxp_port *)data;
+ struct sccnxp_port *s = from_timer(s, t, timer);
unsigned long flags;
spin_lock_irqsave(&s->lock, flags);
@@ -987,8 +987,7 @@ static int sccnxp_probe(struct platform_device *pdev)
dev_err(&pdev->dev, "Unable to reguest IRQ %i\n", s->irq);
} else {
- init_timer(&s->timer);
- setup_timer(&s->timer, sccnxp_timer, (unsigned long)s);
+ timer_setup(&s->timer, sccnxp_timer, 0);
mod_timer(&s->timer, jiffies +
usecs_to_jiffies(s->pdata.poll_time_us));
return 0;
--
2.7.4
--
Kees Cook
Pixel Security
^ permalink raw reply related
* [PATCH] serial: m32r_sio: Drop redundant .data assignment
From: Kees Cook @ 2017-10-24 9:59 UTC (permalink / raw)
To: Greg Kroah-Hartman; +Cc: Jiri Slaby, linux-serial, linux-kernel
With the timer converted to using the new timer_setup()/from_timer() API,
setting the .data field is redundant (and the field will be removed soon),
so drop it.
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: linux-serial@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/tty/serial/m32r_sio.c | 1 -
1 file changed, 1 deletion(-)
diff --git a/drivers/tty/serial/m32r_sio.c b/drivers/tty/serial/m32r_sio.c
index 5f4003ad32b6..0bc548adae52 100644
--- a/drivers/tty/serial/m32r_sio.c
+++ b/drivers/tty/serial/m32r_sio.c
@@ -576,7 +576,6 @@ static int m32r_sio_startup(struct uart_port *port)
timeout = timeout > 6 ? (timeout / 2 - 2) : 1;
- up->timer.data = (unsigned long)up;
mod_timer(&up->timer, jiffies + timeout);
} else {
retval = serial_link_irq_chain(up);
--
2.7.4
--
Kees Cook
Pixel Security
^ permalink raw reply related
* [PATCH] serial: bfin_uart: Convert timers to use timer_setup()
From: Kees Cook @ 2017-10-24 9:59 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jiri Slaby, adi-buildroot-devel, linux-serial, linux-kernel
In preparation for unconditionally passing the struct timer_list pointer to
all timer callbacks, switch to using the new timer_setup() and from_timer()
to pass the timer pointer explicitly.
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: adi-buildroot-devel@lists.sourceforge.net
Cc: linux-serial@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/tty/serial/bfin_uart.c | 7 +++----
1 file changed, 3 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/bfin_uart.c b/drivers/tty/serial/bfin_uart.c
index 293ecbb00684..de5262a46b62 100644
--- a/drivers/tty/serial/bfin_uart.c
+++ b/drivers/tty/serial/bfin_uart.c
@@ -456,8 +456,9 @@ static void bfin_serial_dma_rx_chars(struct bfin_serial_port *uart)
tty_flip_buffer_push(&uart->port.state->port);
}
-void bfin_serial_rx_dma_timeout(struct bfin_serial_port *uart)
+void bfin_serial_rx_dma_timeout(struct timer_list *t)
{
+ struct bfin_serial_port *uart = from_timer(uart, t, rx_dma_timer);
int x_pos, pos;
unsigned long flags;
@@ -624,8 +625,6 @@ static int bfin_serial_startup(struct uart_port *port)
set_dma_start_addr(uart->rx_dma_channel, (unsigned long)uart->rx_dma_buf.buf);
enable_dma(uart->rx_dma_channel);
- uart->rx_dma_timer.data = (unsigned long)(uart);
- uart->rx_dma_timer.function = (void *)bfin_serial_rx_dma_timeout;
uart->rx_dma_timer.expires = jiffies + DMA_RX_FLUSH_JIFFIES;
add_timer(&(uart->rx_dma_timer));
#else
@@ -1316,7 +1315,7 @@ static int bfin_serial_probe(struct platform_device *pdev)
}
uart->rx_dma_channel = res->start;
- init_timer(&(uart->rx_dma_timer));
+ timer_setup(&uart->rx_dma_timer, bfin_serial_rx_dma_timeout, 0);
#endif
#if defined(SERIAL_BFIN_CTSRTS) || \
--
2.7.4
--
Kees Cook
Pixel Security
^ permalink raw reply related
* [PATCH] serial: 8250: Convert timers to use timer_setup()
From: Kees Cook @ 2017-10-24 9:59 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Jiri Slaby, Andy Shevchenko, Vignesh R, Ed Blake,
Matthias Brugger, Sean Young, linux-serial, linux-kernel
In preparation for unconditionally passing the struct timer_list pointer to
all timer callbacks, switch to using the new timer_setup() and from_timer()
to pass the timer pointer explicitly.
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Cc: Jiri Slaby <jslaby@suse.com>
Cc: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Cc: Vignesh R <vigneshr@ti.com>
Cc: Ed Blake <ed.blake@imgtec.com>
Cc: Matthias Brugger <mbrugger@suse.com>
Cc: Sean Young <sean@mess.org>
Cc: linux-serial@vger.kernel.org
Signed-off-by: Kees Cook <keescook@chromium.org>
---
drivers/tty/serial/8250/8250_core.c | 16 +++++++---------
1 file changed, 7 insertions(+), 9 deletions(-)
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 00d4b114f1bf..ccfc42ca846d 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -262,17 +262,17 @@ static void serial_unlink_irq_chain(struct uart_8250_port *up)
* barely passable results for a 16550A. (Although at the expense
* of much CPU overhead).
*/
-static void serial8250_timeout(unsigned long data)
+static void serial8250_timeout(struct timer_list *t)
{
- struct uart_8250_port *up = (struct uart_8250_port *)data;
+ struct uart_8250_port *up = from_timer(up, t, timer);
up->port.handle_irq(&up->port);
mod_timer(&up->timer, jiffies + uart_poll_timeout(&up->port));
}
-static void serial8250_backup_timeout(unsigned long data)
+static void serial8250_backup_timeout(struct timer_list *t)
{
- struct uart_8250_port *up = (struct uart_8250_port *)data;
+ struct uart_8250_port *up = from_timer(up, t, timer);
unsigned int iir, ier = 0, lsr;
unsigned long flags;
@@ -329,8 +329,7 @@ static int univ8250_setup_irq(struct uart_8250_port *up)
if (up->bugs & UART_BUG_THRE) {
pr_debug("ttyS%d - using backup timer\n", serial_index(port));
- up->timer.function = serial8250_backup_timeout;
- up->timer.data = (unsigned long)up;
+ up->timer.function = (TIMER_FUNC_TYPE)serial8250_backup_timeout;
mod_timer(&up->timer, jiffies +
uart_poll_timeout(port) + HZ / 5);
}
@@ -341,7 +340,6 @@ static int univ8250_setup_irq(struct uart_8250_port *up)
* driver used to do this with IRQ0.
*/
if (!port->irq) {
- up->timer.data = (unsigned long)up;
mod_timer(&up->timer, jiffies + uart_poll_timeout(port));
} else
retval = serial_link_irq_chain(up);
@@ -354,7 +352,7 @@ static void univ8250_release_irq(struct uart_8250_port *up)
struct uart_port *port = &up->port;
del_timer_sync(&up->timer);
- up->timer.function = serial8250_timeout;
+ up->timer.function = (TIMER_FUNC_TYPE)serial8250_timeout;
if (port->irq)
serial_unlink_irq_chain(up);
}
@@ -525,7 +523,7 @@ static void __init serial8250_isa_init_ports(void)
base_ops = port->ops;
port->ops = &univ8250_port_ops;
- setup_timer(&up->timer, serial8250_timeout, 0UL);
+ timer_setup(&up->timer, serial8250_timeout, 0);
up->ops = &univ8250_driver_ops;
--
2.7.4
--
Kees Cook
Pixel Security
^ permalink raw reply related
* Re: [PATCH v2 3/3] tty: serial: imx: remove imx_disable_rx_int
From: Juergen Borleis @ 2017-10-23 8:26 UTC (permalink / raw)
To: Uwe Kleine-König
Cc: gregkh, Troy Kisky, linux-serial, nandor.han, fabio.estevam,
linux-arm-kernel, l.stach
In-Reply-To: <20171022184956.exmjlpcbzkvlp7xg@pengutronix.de>
Hi,
On Sunday 22 October 2017 20:49:56 Uwe Kleine-König wrote:
> On Fri, Oct 20, 2017 at 02:25:11PM -0700, Troy Kisky wrote:
> > On 10/20/2017 2:20 PM, Troy Kisky wrote:
> > > Since imx_disable_rx_int is only called by imx_startup,
> > > let's integrate it into that function.
> > >
> > > Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> > > ---
> > > v2: new patch
> > > ---
> > > drivers/tty/serial/imx.c | 39
> > > ++++++++++----------------------------- 1 file changed, 10
> > > insertions(+), 29 deletions(-)
> >
> > While testing this series, I noticed that sending a break on the serial
> > port caused
> >
> > imx-uart 2020000.serial: DMA transaction error.
> >
> > Is that normal ?
>
> I remember that Jürgen (added to To explictly) hit this or a similar
> problem before, maybe he can commen?
Yes, it hit me on i.MX53 after DMA was enabled by default. The driver
currently doesn't handle the "break" event when DMA is active. In this case
the DMA callback returns with an error (that's the message you see, but
it's not an error in this case...), the driver clears the flags and drops
the (break) event itself.
jb
--
Pengutronix e.K. | Juergen Borleis |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH v2 3/3] tty: serial: imx: remove imx_disable_rx_int
From: Uwe Kleine-König @ 2017-10-22 18:49 UTC (permalink / raw)
To: Troy Kisky, Jürgen Borleis
Cc: gregkh, linux-serial, nandor.han, fabio.estevam, linux-arm-kernel,
l.stach
In-Reply-To: <15ff545c-5aaf-4f03-3672-2c1702f78316@boundarydevices.com>
Hello,
On Fri, Oct 20, 2017 at 02:25:11PM -0700, Troy Kisky wrote:
> On 10/20/2017 2:20 PM, Troy Kisky wrote:
> > Since imx_disable_rx_int is only called by imx_startup,
> > let's integrate it into that function.
> >
> > Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> > ---
> > v2: new patch
> > ---
> > drivers/tty/serial/imx.c | 39 ++++++++++-----------------------------
> > 1 file changed, 10 insertions(+), 29 deletions(-)
>
> While testing this series, I noticed that sending a break on the serial port caused
>
> imx-uart 2020000.serial: DMA transaction error.
>
> Is that normal ?
I remember that Jürgen (added to To explictly) hit this or a similar
problem before, maybe he can commen?
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | http://www.pengutronix.de/ |
^ permalink raw reply
* Re: [PATCH v8 2/5] serdev: Introduce devm_serdev_device_open()
From: Guenter Roeck @ 2017-10-21 17:09 UTC (permalink / raw)
To: Andrey Smirnov, linux-kernel
Cc: linux-serial, Rob Herring, cphealy, Lucas Stach,
Nikita Yushchenko, Lee Jones, Greg Kroah-Hartman, Pavel Machek,
Andy Shevchenko, Johan Hovold
In-Reply-To: <20171018170136.12347-3-andrew.smirnov@gmail.com>
On 10/18/2017 10:01 AM, Andrey Smirnov wrote:
> Add code implementing managed version of serdev_device_open() for
> serdev device drivers that "open" the device during driver's lifecycle
> only once (e.g. opened in .probe() and closed in .remove()).
>
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-serial@vger.kernel.org
> Cc: Rob Herring <robh@kernel.org>
> Cc: cphealy@gmail.com
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Johan Hovold <johan@kernel.org>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> ---
> Documentation/driver-model/devres.txt | 3 +++
> drivers/tty/serdev/core.c | 27 +++++++++++++++++++++++++++
> include/linux/serdev.h | 1 +
> 3 files changed, 31 insertions(+)
>
> diff --git a/Documentation/driver-model/devres.txt b/Documentation/driver-model/devres.txt
> index 69f08c0f23a8..e9c6b5cfeec1 100644
> --- a/Documentation/driver-model/devres.txt
> +++ b/Documentation/driver-model/devres.txt
> @@ -383,6 +383,9 @@ RESET
> devm_reset_control_get()
> devm_reset_controller_register()
>
> +SERDEV
> + devm_serdev_device_open()
> +
> SLAVE DMA ENGINE
> devm_acpi_dma_controller_register()
>
> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index f500f6a2ca88..b3a785665c6f 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
> @@ -116,6 +116,33 @@ void serdev_device_close(struct serdev_device *serdev)
> }
> EXPORT_SYMBOL_GPL(serdev_device_close);
>
> +static void devm_serdev_device_release(struct device *dev, void *dr)
> +{
> + serdev_device_close(*(struct serdev_device **)dr);
> +}
> +
> +int devm_serdev_device_open(struct device *dev, struct serdev_device *serdev)
> +{
> + struct serdev_device **dr;
> + int ret;
> +
> + dr = devres_alloc(devm_serdev_device_release, sizeof(*dr), GFP_KERNEL);
> + if (!dr)
> + return -ENOMEM;
> +
> + ret = serdev_device_open(serdev);
> + if (ret) {
> + devres_free(dr);
> + return ret;
> + }
> +
> + *dr = serdev;
> + devres_add(dev, dr);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(devm_serdev_device_open);
> +
> void serdev_device_write_wakeup(struct serdev_device *serdev)
> {
> complete(&serdev->write_comp);
> diff --git a/include/linux/serdev.h b/include/linux/serdev.h
> index e69402d4a8ae..9929063bd45d 100644
> --- a/include/linux/serdev.h
> +++ b/include/linux/serdev.h
> @@ -193,6 +193,7 @@ static inline int serdev_controller_receive_buf(struct serdev_controller *ctrl,
>
> int serdev_device_open(struct serdev_device *);
> void serdev_device_close(struct serdev_device *);
> +int devm_serdev_device_open(struct device *, struct serdev_device *);
> unsigned int serdev_device_set_baudrate(struct serdev_device *, unsigned int);
> void serdev_device_set_flow_control(struct serdev_device *, bool);
> int serdev_device_write_buf(struct serdev_device *, const unsigned char *, size_t);
>
^ permalink raw reply
* Re: [PATCH v8 1/5] serdev: Make .remove in struct serdev_device_driver optional
From: Guenter Roeck @ 2017-10-21 17:08 UTC (permalink / raw)
To: Andrey Smirnov, linux-kernel
Cc: linux-serial, Rob Herring, cphealy, Lucas Stach,
Nikita Yushchenko, Lee Jones, Greg Kroah-Hartman, Pavel Machek,
Andy Shevchenko, Johan Hovold
In-Reply-To: <20171018170136.12347-2-andrew.smirnov@gmail.com>
On 10/18/2017 10:01 AM, Andrey Smirnov wrote:
> Using devres infrastructure it is possible to wirte a serdev driver
s/wirte/write/
otherwise
Reviewed-by: Guenter Roeck <linux@roeck-us.net>
> that doesn't have any code that needs to be called as a part of
> .remove. Add code to make .remove optional.
>
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-serial@vger.kernel.org
> Cc: Rob Herring <robh@kernel.org>
> Cc: cphealy@gmail.com
> Cc: Guenter Roeck <linux@roeck-us.net>
> Cc: Lucas Stach <l.stach@pengutronix.de>
> Cc: Nikita Yushchenko <nikita.yoush@cogentembedded.com>
> Cc: Lee Jones <lee.jones@linaro.org>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Cc: Pavel Machek <pavel@ucw.cz>
> Cc: Andy Shevchenko <andy.shevchenko@gmail.com>
> Cc: Johan Hovold <johan@kernel.org>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> ---
> drivers/tty/serdev/core.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serdev/core.c b/drivers/tty/serdev/core.c
> index c68fb3a8ea1c..f500f6a2ca88 100644
> --- a/drivers/tty/serdev/core.c
> +++ b/drivers/tty/serdev/core.c
> @@ -252,8 +252,8 @@ static int serdev_drv_probe(struct device *dev)
> static int serdev_drv_remove(struct device *dev)
> {
> const struct serdev_device_driver *sdrv = to_serdev_device_driver(dev->driver);
> -
> - sdrv->remove(to_serdev_device(dev));
> + if (sdrv->remove)
> + sdrv->remove(to_serdev_device(dev));
> return 0;
> }
>
>
^ permalink raw reply
* Re: [PATCH v3 0/2] ACPI serdev support
From: Johan Hovold @ 2017-10-21 9:59 UTC (permalink / raw)
To: Marcel Holtmann
Cc: Hans de Goede, Loic Poulain, Johan Hovold, Greg Kroah-Hartman,
Frédéric Danis, Rafael J. Wysocki, Rob Herring,
Sebastian Reichel, Loic Poulain, Lukas Wunner,
open list:BLUETOOTH DRIVERS, linux-serial@vger.kernel.org,
ACPI Devel Maling List
In-Reply-To: <FFD85CA6-6BD9-45AA-B3C2-898212711C0C@holtmann.org>
On Thu, Oct 19, 2017 at 09:00:25PM +0200, Marcel Holtmann wrote:
> Hi Hans,
>
> >> Hmm, I've never actually seen any hardware use an intel BT HCI connected
> >> to a serdev, but I guess people did not write that code for fun, so those
> >> do exist ?
> >> they are all ACPI based and could now start using serdev. Previously they were all driven by btattach.
> >> I understand, I was just wondering if anyone is aware of any hardware
> >> actually using Intel BT devices in this manner, because it is going
> >> to be tricky to do a similar series if we cannot test it.
> >> Yeah this driver supports Lightning-Peak iBT 3.0 UART controller.
> >> I remember this controller comes (at least) with Atom x3-c3440 / x3-c3445 SoCs.
> >> However I don't have the hardware anymore.
> >
> > Ah, those are the Rockchip manufactured Atom based SoCs which
> > come with Mali graphics, I don't think anyone is trying to run
> > mainline on those and not a whole lot of those have shipped to
> > begin with (the line has been canceled).
> >
> > I've an Apollo Lake based laptop with Intel Bluetooth, I just
> > checked and that is simply using USB.
> >
> > So I don't think we are actually going to break anything by
> > just moving forward as planned. This reminds me of the axp288
> > driver where Intel's "embedded" engineering team upstreamed
> > parts of the axp288 PMIC code, but in a way where the driver
> > could not work as it would not load with platform_data which
> > was never provided by the mainline kernel.
> >
> > As such it might actually be good to break this and if no-one
> > complains we can just remove the hci_intel.c code altogether.
>
> I think we can go forward with what we have. I am not going to remove
> hci_intel.c since the firmware loading code etc. is pretty generic to
> all Intel UART based chips. However we just take the PM pieces out or
> maybe someone is going to fix them.
Note that hci_intel used the platform hack for the reset line and host
wake-up irq. And as the reset line appears to be required when using
ACPI, we'd definitely find out eventually if anyone is using such
devices as they would fail to even power on.
> > The alternative would be to revert 2 if the 3 patches of
> > my last series for making the BT on the GPD win / pocket
> > (and likely other devices) work in uart mode. Then someone
> > (me?) would need to do a completely untested series for
> > hci_intel.c, and get that + reverts of the reverts into 4.16,
> > which is not the best way forward IMHO.
>
> I am not doing that. We have to make progress towards serdev. So if
> things really turn out badly, then we have to deal with it, but that
> should not hold up getting things in the direction this needs to go.
As long as we know what we are breaking.
Johan
^ permalink raw reply
* Re: [PATCH 1/1] tty: serial: imx: allow breaks to be received when using dma
From: Troy Kisky @ 2017-10-20 22:17 UTC (permalink / raw)
To: gregkh, nandor.han
Cc: fabio.estevam, l.stach, linux-arm-kernel, linux-serial,
u.kleine-koenig
In-Reply-To: <20171020221340.14974-1-troy.kisky@boundarydevices.com>
On 10/20/2017 3:13 PM, Troy Kisky wrote:
> This allows me to login after sending a break when service
> serial-getty@ttymxc0.service is running
>
> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> ---
> drivers/tty/serial/imx.c | 20 +++++++++++++-------
> 1 file changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
> index 506fcd742b47..39033f460a24 100644
> --- a/drivers/tty/serial/imx.c
> +++ b/drivers/tty/serial/imx.c
> @@ -934,7 +934,6 @@ static void dma_rx_callback(void *data)
> status = dmaengine_tx_status(chan, (dma_cookie_t)0, &state);
>
> if (status == DMA_ERROR) {
> - dev_err(sport->port.dev, "DMA transaction error.\n");
> clear_rx_errors(sport);
> return;
> }
> @@ -1035,6 +1034,7 @@ static int start_rx_dma(struct imx_port *sport)
>
> static void clear_rx_errors(struct imx_port *sport)
> {
> + struct tty_port *port = &sport->port.state->port;
> unsigned int status_usr1, status_usr2;
>
> status_usr1 = readl(sport->port.membase + USR1);
> @@ -1043,12 +1043,18 @@ static void clear_rx_errors(struct imx_port *sport)
> if (status_usr2 & USR2_BRCD) {
> sport->port.icount.brk++;
> writel(USR2_BRCD, sport->port.membase + USR2);
> - } else if (status_usr1 & USR1_FRAMERR) {
> - sport->port.icount.frame++;
> - writel(USR1_FRAMERR, sport->port.membase + USR1);
> - } else if (status_usr1 & USR1_PARITYERR) {
> - sport->port.icount.parity++;
> - writel(USR1_PARITYERR, sport->port.membase + USR1);
> + if (tty_insert_flip_char(port, 0, TTY_BREAK) == 0)
> + sport->port.icount.buf_overrun++;
> + tty_flip_buffer_push(port);
> + } else {
> + dev_err(sport->port.dev, "DMA transaction error.\n");
> + if (status_usr1 & USR1_FRAMERR) {
> + sport->port.icount.frame++;
> + writel(USR1_FRAMERR, sport->port.membase + USR1);
> + } else if (status_usr1 & USR1_PARITYERR) {
> + sport->port.icount.parity++;
> + writel(USR1_PARITYERR, sport->port.membase + USR1);
> + }
> }
>
> if (status_usr2 & USR2_ORE) {
>
Does this need to use
spin_lock_irqsave(&sport->port.lock, flags);
I would have, but dma_rx_callback doesn't.
BR
Troy
^ permalink raw reply
* [PATCH 1/1] tty: serial: imx: allow breaks to be received when using dma
From: Troy Kisky @ 2017-10-20 22:13 UTC (permalink / raw)
To: gregkh, nandor.han
Cc: Troy Kisky, linux-serial, u.kleine-koenig, fabio.estevam,
linux-arm-kernel, l.stach
This allows me to login after sending a break when service
serial-getty@ttymxc0.service is running
Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
drivers/tty/serial/imx.c | 20 +++++++++++++-------
1 file changed, 13 insertions(+), 7 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 506fcd742b47..39033f460a24 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -934,7 +934,6 @@ static void dma_rx_callback(void *data)
status = dmaengine_tx_status(chan, (dma_cookie_t)0, &state);
if (status == DMA_ERROR) {
- dev_err(sport->port.dev, "DMA transaction error.\n");
clear_rx_errors(sport);
return;
}
@@ -1035,6 +1034,7 @@ static int start_rx_dma(struct imx_port *sport)
static void clear_rx_errors(struct imx_port *sport)
{
+ struct tty_port *port = &sport->port.state->port;
unsigned int status_usr1, status_usr2;
status_usr1 = readl(sport->port.membase + USR1);
@@ -1043,12 +1043,18 @@ static void clear_rx_errors(struct imx_port *sport)
if (status_usr2 & USR2_BRCD) {
sport->port.icount.brk++;
writel(USR2_BRCD, sport->port.membase + USR2);
- } else if (status_usr1 & USR1_FRAMERR) {
- sport->port.icount.frame++;
- writel(USR1_FRAMERR, sport->port.membase + USR1);
- } else if (status_usr1 & USR1_PARITYERR) {
- sport->port.icount.parity++;
- writel(USR1_PARITYERR, sport->port.membase + USR1);
+ if (tty_insert_flip_char(port, 0, TTY_BREAK) == 0)
+ sport->port.icount.buf_overrun++;
+ tty_flip_buffer_push(port);
+ } else {
+ dev_err(sport->port.dev, "DMA transaction error.\n");
+ if (status_usr1 & USR1_FRAMERR) {
+ sport->port.icount.frame++;
+ writel(USR1_FRAMERR, sport->port.membase + USR1);
+ } else if (status_usr1 & USR1_PARITYERR) {
+ sport->port.icount.parity++;
+ writel(USR1_PARITYERR, sport->port.membase + USR1);
+ }
}
if (status_usr2 & USR2_ORE) {
--
2.11.0
^ permalink raw reply related
* Re: [PATCH 1/2] serdev: ttyport: enforce tty-driver open() requirement
From: Rob Herring @ 2017-10-20 21:43 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Johan Hovold, Jiri Slaby, linux-serial@vger.kernel.org,
linux-kernel@vger.kernel.org
In-Reply-To: <20171020122144.GA30326@kroah.com>
On Fri, Oct 20, 2017 at 7:21 AM, Greg Kroah-Hartman
<gregkh@linuxfoundation.org> wrote:
> On Tue, Oct 17, 2017 at 11:07:20AM -0500, Rob Herring wrote:
>> On Mon, Oct 16, 2017 at 8:06 AM, Johan Hovold <johan@kernel.org> wrote:
>> > The tty-driver open routine is mandatory, but the serdev
>> > tty-port-controller implementation did not treat it as such and would
>> > instead fall back to calling tty_port_open() directly.
>>
>> The idea was to eventually get rid of the tty_struct dependency and
>> only depend on tty_port. That's very invasive though and needs various
>> pieces of tty_struct to move into tty_port.
>>
>> Of course, tty_port_open itself would have to change as well, so this
>> change doesn't really matter.
>
> So are you acking these patches?
Yeah, I guess. I was mainly trying to give some background on why it
was done the way it was.
For both:
Acked-by: Rob Herring <robh@kernel.org>
Rob
^ permalink raw reply
* Re: [PATCH v2 3/3] tty: serial: imx: remove imx_disable_rx_int
From: Troy Kisky @ 2017-10-20 21:25 UTC (permalink / raw)
To: gregkh, nandor.han
Cc: fabio.estevam, l.stach, linux-arm-kernel, linux-serial,
u.kleine-koenig
In-Reply-To: <20171020212021.13065-3-troy.kisky@boundarydevices.com>
On 10/20/2017 2:20 PM, Troy Kisky wrote:
> Since imx_disable_rx_int is only called by imx_startup,
> let's integrate it into that function.
>
> Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
> ---
> v2: new patch
> ---
> drivers/tty/serial/imx.c | 39 ++++++++++-----------------------------
> 1 file changed, 10 insertions(+), 29 deletions(-)
While testing this series, I noticed that sending a break on the serial port caused
imx-uart 2020000.serial: DMA transaction error.
Is that normal ?
^ permalink raw reply
* [PATCH v2 3/3] tty: serial: imx: remove imx_disable_rx_int
From: Troy Kisky @ 2017-10-20 21:20 UTC (permalink / raw)
To: gregkh, nandor.han
Cc: Troy Kisky, linux-serial, u.kleine-koenig, fabio.estevam,
linux-arm-kernel, l.stach
In-Reply-To: <20171020212021.13065-1-troy.kisky@boundarydevices.com>
Since imx_disable_rx_int is only called by imx_startup,
let's integrate it into that function.
Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
v2: new patch
---
drivers/tty/serial/imx.c | 39 ++++++++++-----------------------------
1 file changed, 10 insertions(+), 29 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index 15b0ecb4cf60..506fcd742b47 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -710,27 +710,6 @@ static irqreturn_t imx_rxint(int irq, void *dev_id)
return IRQ_HANDLED;
}
-static void imx_disable_rx_int(struct imx_port *sport)
-{
- unsigned long temp;
-
- sport->dma_is_rxing = 1;
-
- /* disable the receiver ready and aging timer interrupts */
- temp = readl(sport->port.membase + UCR1);
- temp &= ~(UCR1_RRDYEN);
- writel(temp, sport->port.membase + UCR1);
-
- temp = readl(sport->port.membase + UCR2);
- temp &= ~(UCR2_ATEN);
- writel(temp, sport->port.membase + UCR2);
-
- /* disable the rx errors interrupts */
- temp = readl(sport->port.membase + UCR4);
- temp &= ~UCR4_OREN;
- writel(temp, sport->port.membase + UCR4);
-}
-
static void clear_rx_errors(struct imx_port *sport);
/*
@@ -1024,6 +1003,7 @@ static int start_rx_dma(struct imx_port *sport)
struct dma_async_tx_descriptor *desc;
int ret;
+ sport->dma_is_rxing = 1;
sport->rx_ring.head = 0;
sport->rx_ring.tail = 0;
sport->rx_periods = RX_DMA_PERIODS;
@@ -1260,18 +1240,21 @@ static int imx_startup(struct uart_port *port)
if (sport->dma_is_inited && !sport->dma_is_enabled)
imx_enable_dma(sport);
- temp = readl(sport->port.membase + UCR1);
- temp |= UCR1_RRDYEN | UCR1_UARTEN;
+ temp = readl(sport->port.membase + UCR1) & ~UCR1_RRDYEN;
+ if (!sport->dma_is_enabled)
+ temp |= UCR1_RRDYEN;
+ temp |= UCR1_UARTEN;
if (sport->have_rtscts)
temp |= UCR1_RTSDEN;
writel(temp, sport->port.membase + UCR1);
- temp = readl(sport->port.membase + UCR4);
- temp |= UCR4_OREN;
+ temp = readl(sport->port.membase + UCR4) & ~UCR4_OREN;
+ if (!sport->dma_is_enabled)
+ temp |= UCR4_OREN;
writel(temp, sport->port.membase + UCR4);
- temp = readl(sport->port.membase + UCR2);
+ temp = readl(sport->port.membase + UCR2) & ~UCR2_ATEN;
temp |= (UCR2_RXEN | UCR2_TXEN);
if (!sport->have_rtscts)
temp |= UCR2_IRTS;
@@ -1305,10 +1288,8 @@ static int imx_startup(struct uart_port *port)
* In our iMX53 the average delay for the first reception dropped from
* approximately 35000 microseconds to 1000 microseconds.
*/
- if (sport->dma_is_enabled) {
- imx_disable_rx_int(sport);
+ if (sport->dma_is_enabled)
start_rx_dma(sport);
- }
spin_unlock_irqrestore(&sport->port.lock, flags);
--
2.11.0
^ permalink raw reply related
* [PATCH v2 2/3] tty: serial: imx: remove dead code imx_dma_rxint
From: Troy Kisky @ 2017-10-20 21:20 UTC (permalink / raw)
To: gregkh, nandor.han
Cc: Troy Kisky, linux-serial, u.kleine-koenig, fabio.estevam,
linux-arm-kernel, l.stach
In-Reply-To: <20171020212021.13065-1-troy.kisky@boundarydevices.com>
Since commit 4dec2f119e86 ("imx-serial: RX DMA startup latency")
the interrupt routine no longer will start rx dma.
imx_dma_rxint no longer needs to be called to try and start dma.
It won't start dma because dma_is_rxing is
already true meaning dma is already started.
Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
v2: split into 2 patches
---
drivers/tty/serial/imx.c | 30 ++----------------------------
1 file changed, 2 insertions(+), 28 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index bf47b15df2e8..15b0ecb4cf60 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -732,29 +732,6 @@ static void imx_disable_rx_int(struct imx_port *sport)
}
static void clear_rx_errors(struct imx_port *sport);
-static int start_rx_dma(struct imx_port *sport);
-/*
- * If the RXFIFO is filled with some data, and then we
- * arise a DMA operation to receive them.
- */
-static void imx_dma_rxint(struct imx_port *sport)
-{
- unsigned long temp;
- unsigned long flags;
-
- spin_lock_irqsave(&sport->port.lock, flags);
-
- temp = readl(sport->port.membase + USR2);
- if ((temp & USR2_RDR) && !sport->dma_is_rxing) {
-
- imx_disable_rx_int(sport);
-
- /* tell the DMA to receive the data. */
- start_rx_dma(sport);
- }
-
- spin_unlock_irqrestore(&sport->port.lock, flags);
-}
/*
* We have a modem side uart, so the meanings of RTS and CTS are inverted.
@@ -816,11 +793,8 @@ static irqreturn_t imx_int(int irq, void *dev_id)
sts = readl(sport->port.membase + USR1);
sts2 = readl(sport->port.membase + USR2);
- if (sts & (USR1_RRDY | USR1_AGTIM)) {
- if (sport->dma_is_enabled)
- imx_dma_rxint(sport);
- else
- imx_rxint(irq, dev_id);
+ if (!sport->dma_is_enabled && (sts & (USR1_RRDY | USR1_AGTIM))) {
+ imx_rxint(irq, dev_id);
ret = IRQ_HANDLED;
}
--
2.11.0
^ permalink raw reply related
* [PATCH v2 1/3] tty: serial: imx: disable ageing timer interrupt if dma in use
From: Troy Kisky @ 2017-10-20 21:20 UTC (permalink / raw)
To: gregkh, nandor.han
Cc: Troy Kisky, linux-serial, u.kleine-koenig, fabio.estevam,
linux-arm-kernel, l.stach
Since commit 4dec2f119e86 ("imx-serial: RX DMA startup latency")
the interrupt routine no longer will start rx dma.
So, we no longer need to enable this interrupt to start dma.
Signed-off-by: Troy Kisky <troy.kisky@boundarydevices.com>
---
v2: split into a series of 2 patches
---
drivers/tty/serial/imx.c | 4 ----
1 file changed, 4 deletions(-)
diff --git a/drivers/tty/serial/imx.c b/drivers/tty/serial/imx.c
index dfeff3951f93..bf47b15df2e8 100644
--- a/drivers/tty/serial/imx.c
+++ b/drivers/tty/serial/imx.c
@@ -1207,10 +1207,6 @@ static void imx_enable_dma(struct imx_port *sport)
temp |= UCR1_RDMAEN | UCR1_TDMAEN | UCR1_ATDMAEN;
writel(temp, sport->port.membase + UCR1);
- temp = readl(sport->port.membase + UCR2);
- temp |= UCR2_ATEN;
- writel(temp, sport->port.membase + UCR2);
-
imx_setup_ufcr(sport, TXTL_DMA, RXTL_DMA);
sport->dma_is_enabled = 1;
--
2.11.0
^ permalink raw reply related
* Re: [patch v9 1/4] drivers: jtag: Add JTAG core driver
From: Greg KH @ 2017-10-20 14:54 UTC (permalink / raw)
To: Oleksandr Shamray
Cc: arnd@arndb.de, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
openbmc@lists.ozlabs.org, joel@jms.id.au, jiri@resnulli.us,
tklauser@distanz.ch, linux-serial@vger.kernel.org, mec@shout.net,
Vadim Pasternak, system-sw-low-level, robh+dt@kernel.org,
openocd-devel-owner@lists.sourceforge.net
In-Reply-To: <AM4PR0501MB21940531802FC14EDE8C3F75B1430@AM4PR0501MB2194.eurprd05.prod.outlook.com>
On Fri, Oct 20, 2017 at 02:34:00PM +0000, Oleksandr Shamray wrote:
> Hi Greg.
>
> > -----Original Message-----
> > From: Greg KH [mailto:gregkh@linuxfoundation.org]
> > Sent: Friday, October 20, 2017 2:55 PM
> > To: Oleksandr Shamray <oleksandrs@mellanox.com>
> > Cc: arnd@arndb.de; linux-kernel@vger.kernel.org; linux-arm-
> > kernel@lists.infradead.org; devicetree@vger.kernel.org;
> > openbmc@lists.ozlabs.org; joel@jms.id.au; jiri@resnulli.us;
> > tklauser@distanz.ch; linux-serial@vger.kernel.org; mec@shout.net; Vadim
> > Pasternak <vadimp@mellanox.com>; system-sw-low-level <system-sw-low-
> > level@mellanox.com>; robh+dt@kernel.org; openocd-devel-
> > owner@lists.sourceforge.net; linux-api@vger.kernel.org;
> > davem@davemloft.net; mchehab@kernel.org; Jiri Pirko <jiri@mellanox.com>
> > Subject: Re: [patch v9 1/4] drivers: jtag: Add JTAG core driver
> >
> > On Thu, Sep 21, 2017 at 12:25:29PM +0300, Oleksandr Shamray wrote:
> > > +struct jtag {
> > > + struct device *dev;
> > > + struct cdev cdev;
> >
> > Why are you using a cdev here and not just a normal misc device?
>
> What the benefits to use misc instead of cdev?
Less code, simpler logic, easier to review and understand, etc.
Let me ask you, why use a cdev instead of a misc?
> > I forgot if this is what you were doing before, sorry...
> >
> > > + int id;
> > > + atomic_t open;
> >
> > Why do you need this?
>
> This counter used to avoid open at the same time by 2 or more users.
But it isn't working :)
And why do you care?
> > > + const struct jtag_ops *ops;
> > > + unsigned long priv[0] __aligned(ARCH_DMA_MINALIGN);
> >
> > Huh? Why is this needed to be dma aligned? Why not just use the private
> > pointer in struct device?
> >
>
> It is critical?
You are saying it is, so you have to justify it. There is a pointer for
you to use, don't make new ones for no reason, right?
> > > +};
> > > +
> > > +static dev_t jtag_devt;
> > > +static DEFINE_IDA(jtag_ida);
> > > +
> > > +void *jtag_priv(struct jtag *jtag)
> > > +{
> > > + return jtag->priv;
> > > +}
> > > +EXPORT_SYMBOL_GPL(jtag_priv);
> > > +
> > > +static u8 *jtag_copy_from_user(__u64 udata, unsigned long bit_size) {
> > > + unsigned long size;
> > > + void *kdata;
> > > +
> > > + size = DIV_ROUND_UP(bit_size, BITS_PER_BYTE);
> > > + kdata = memdup_user(u64_to_user_ptr(udata), size);
> >
> > You only use this once, why not just open-code it?
>
> I think it makes code more understandable.
As a reviewer, I don't :)
> > > +
> > > + return kdata;
> > > +}
> > > +
> > > +static unsigned long jtag_copy_to_user(__u64 udata, u8 *kdata,
> > > + unsigned long bit_size)
> > > +{
> > > + unsigned long size;
> > > +
> > > + size = DIV_ROUND_UP(bit_size, BITS_PER_BYTE);
> > > +
> > > + return copy_to_user(u64_to_user_ptr(udata), (void *)(kdata), size);
> >
> > Same here, making this a separate function seems odd.
>
> Same, I think it makes code more understandable.
But it doesn't.
> > > +
> > > + if (jtag->ops->freq_set)
> > > + err = jtag->ops->freq_set(jtag, value);
> > > + else
> > > + err = -EOPNOTSUPP;
> > > + break;
> > > +
> > > + case JTAG_IOCRUNTEST:
> > > + if (copy_from_user(&idle, (void *)arg,
> > > + sizeof(struct jtag_run_test_idle)))
> > > + return -ENOMEM;
> > > + err = jtag_run_test_idle_op(jtag, &idle);
> >
> > Who validates the structure fields? Is that up to the individual jtag driver? Why
> > not do it in the core correctly so that it only has to be done in one place and you
> > do not have to audit every individual driver?
>
> Input parameters validated by jtag platform driver. I think it not critical.
Not true at all. It is very critical. Remmeber, "All Input Is Evil!"
You have to validate this. I as a reviewer have to find where you are
validating this data to ensure bad things do not happen. I can't review
that here, now I have to go and review all of the individual drivers,
which is a major pain, don't you agree?
> > > + break;
> > > +
> > > + case JTAG_IOCXFER:
> > > + if (copy_from_user(&xfer, (void *)arg,
> > > + sizeof(struct jtag_xfer)))
> > > + return -EFAULT;
> > > +
> > > + if (xfer.length >= JTAG_MAX_XFER_DATA_LEN)
> > > + return -EFAULT;
> > > +
> > > + xfer_data = jtag_copy_from_user(xfer.tdio, xfer.length);
> > > + if (!xfer_data)
> > > + return -ENOMEM;
> >
> > Are you sure that's the correct error value?
>
> I think yes, but what you suggest?
A fault happened, so -EFAULT, right?
> [..]
> > + .unlocked_ioctl = jtag_ioctl,
> > + .open = jtag_open,
> > + .release = jtag_release,
> > +};
>
> add a compat_ioctl pointer here, after ensuring that all ioctl
> commands are compatible between 32-bit and 64-bit user space.
> [..]
And if you do not, what happens? You shouldn't need it as there is no
fixups necessary, or am I mistaken about that?
> > > +static int jtag_open(struct inode *inode, struct file *file) {
> > > + struct jtag *jtag = container_of(inode->i_cdev, struct jtag, cdev);
> > > +
> > > + if (atomic_read(&jtag->open)) {
> > > + dev_info(NULL, "jtag already opened\n");
> > > + return -EBUSY;
> >
> > Why do you care if multiple opens can happen?
>
> Jtag HW not support to using with multiple requests from different users. So we prohibit this.
Why does the kernel care?
And again, your implementation is broken, it's not actually doing this
protection. I recommend just not doing it at all, but if you really are
insisting on it, you have to get it correct :)
thanks,
greg k-h
^ permalink raw reply
* RE: [patch v9 1/4] drivers: jtag: Add JTAG core driver
From: Oleksandr Shamray @ 2017-10-20 14:34 UTC (permalink / raw)
To: Greg KH
Cc: arnd@arndb.de, linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org, devicetree@vger.kernel.org,
openbmc@lists.ozlabs.org, joel@jms.id.au, jiri@resnulli.us,
tklauser@distanz.ch, linux-serial@vger.kernel.org, mec@shout.net,
Vadim Pasternak, system-sw-low-level, robh+dt@kernel.org,
openocd-devel-owner@lists.sourceforge.net
In-Reply-To: <20171020115512.GA2073@kroah.com>
Hi Greg.
> -----Original Message-----
> From: Greg KH [mailto:gregkh@linuxfoundation.org]
> Sent: Friday, October 20, 2017 2:55 PM
> To: Oleksandr Shamray <oleksandrs@mellanox.com>
> Cc: arnd@arndb.de; linux-kernel@vger.kernel.org; linux-arm-
> kernel@lists.infradead.org; devicetree@vger.kernel.org;
> openbmc@lists.ozlabs.org; joel@jms.id.au; jiri@resnulli.us;
> tklauser@distanz.ch; linux-serial@vger.kernel.org; mec@shout.net; Vadim
> Pasternak <vadimp@mellanox.com>; system-sw-low-level <system-sw-low-
> level@mellanox.com>; robh+dt@kernel.org; openocd-devel-
> owner@lists.sourceforge.net; linux-api@vger.kernel.org;
> davem@davemloft.net; mchehab@kernel.org; Jiri Pirko <jiri@mellanox.com>
> Subject: Re: [patch v9 1/4] drivers: jtag: Add JTAG core driver
>
> On Thu, Sep 21, 2017 at 12:25:29PM +0300, Oleksandr Shamray wrote:
> > +struct jtag {
> > + struct device *dev;
> > + struct cdev cdev;
>
> Why are you using a cdev here and not just a normal misc device?
What the benefits to use misc instead of cdev?
> I forgot if this is what you were doing before, sorry...
>
> > + int id;
> > + atomic_t open;
>
> Why do you need this?
This counter used to avoid open at the same time by 2 or more users.
>
> > + const struct jtag_ops *ops;
> > + unsigned long priv[0] __aligned(ARCH_DMA_MINALIGN);
>
> Huh? Why is this needed to be dma aligned? Why not just use the private
> pointer in struct device?
>
It is critical?
> > +};
> > +
> > +static dev_t jtag_devt;
> > +static DEFINE_IDA(jtag_ida);
> > +
> > +void *jtag_priv(struct jtag *jtag)
> > +{
> > + return jtag->priv;
> > +}
> > +EXPORT_SYMBOL_GPL(jtag_priv);
> > +
> > +static u8 *jtag_copy_from_user(__u64 udata, unsigned long bit_size) {
> > + unsigned long size;
> > + void *kdata;
> > +
> > + size = DIV_ROUND_UP(bit_size, BITS_PER_BYTE);
> > + kdata = memdup_user(u64_to_user_ptr(udata), size);
>
> You only use this once, why not just open-code it?
I think it makes code more understandable.
>
> > +
> > + return kdata;
> > +}
> > +
> > +static unsigned long jtag_copy_to_user(__u64 udata, u8 *kdata,
> > + unsigned long bit_size)
> > +{
> > + unsigned long size;
> > +
> > + size = DIV_ROUND_UP(bit_size, BITS_PER_BYTE);
> > +
> > + return copy_to_user(u64_to_user_ptr(udata), (void *)(kdata), size);
>
> Same here, making this a separate function seems odd.
Same, I think it makes code more understandable.
>
> > +}
> > +
> > +static struct class jtag_class = {
> > + .name = "jtag",
> > + .owner = THIS_MODULE,
> > +};
> > +
> > +static int jtag_run_test_idle_op(struct jtag *jtag,
> > + struct jtag_run_test_idle *idle) {
> > + if (jtag->ops->idle)
> > + return jtag->ops->idle(jtag, idle);
> > + else
> > + return -EOPNOTSUPP;
>
> Why is this a function? Why not just do this work in the ioctl?
Agree. I will move it just to ioctl function body.
>
> > +}
> > +
> > +static int jtag_xfer_op(struct jtag *jtag, struct jtag_xfer *xfer, u8
> > +*data) {
> > + if (jtag->ops->xfer)
> > + return jtag->ops->xfer(jtag, xfer, data);
> > + else
> > + return -EOPNOTSUPP;
>
> Same here, doesn't need to be a function.
Agree.
>
> > +}
> > +
> > +static long jtag_ioctl(struct file *file, unsigned int cmd, unsigned
> > +long arg) {
> > + struct jtag *jtag = file->private_data;
> > + struct jtag_run_test_idle idle;
> > + struct jtag_xfer xfer;
> > + u8 *xfer_data;
> > + u32 value;
> > + int err;
> > +
> > + switch (cmd) {
> > + case JTAG_GIOCFREQ:
> > + if (jtag->ops->freq_get)
> > + err = jtag->ops->freq_get(jtag, &value);
> > + else
> > + err = -EOPNOTSUPP;
> > + if (err)
> > + break;
> > +
> > + err = put_user(value, (__u32 *)arg);
> > + break;
>
> Are you sure the return value of put_user is correct here? Shouldn't you return
> -EFAULT if err is not 0?
Yes, thanks for point of this.
>
> > +
> > + case JTAG_SIOCFREQ:
> > + err = get_user(value, (__u32 *)arg);
> > +
>
> why a blank line?
Will remove
>
> > + if (value == 0)
> > + err = -EINVAL;
>
> Check err first.
Ok.
>
> > + if (err)
> > + break;
>
> And set it properly, again err should be -EFAULT, right?
Right 😊0
>
> > +
> > + if (jtag->ops->freq_set)
> > + err = jtag->ops->freq_set(jtag, value);
> > + else
> > + err = -EOPNOTSUPP;
> > + break;
> > +
> > + case JTAG_IOCRUNTEST:
> > + if (copy_from_user(&idle, (void *)arg,
> > + sizeof(struct jtag_run_test_idle)))
> > + return -ENOMEM;
> > + err = jtag_run_test_idle_op(jtag, &idle);
>
> Who validates the structure fields? Is that up to the individual jtag driver? Why
> not do it in the core correctly so that it only has to be done in one place and you
> do not have to audit every individual driver?
Input parameters validated by jtag platform driver. I think it not critical.
>
> > + break;
> > +
> > + case JTAG_IOCXFER:
> > + if (copy_from_user(&xfer, (void *)arg,
> > + sizeof(struct jtag_xfer)))
> > + return -EFAULT;
> > +
> > + if (xfer.length >= JTAG_MAX_XFER_DATA_LEN)
> > + return -EFAULT;
> > +
> > + xfer_data = jtag_copy_from_user(xfer.tdio, xfer.length);
> > + if (!xfer_data)
> > + return -ENOMEM;
>
> Are you sure that's the correct error value?
I think yes, but what you suggest?
>
> > +
> > + err = jtag_xfer_op(jtag, &xfer, xfer_data);
> > + if (jtag_copy_to_user(xfer.tdio, xfer_data, xfer.length)) {
> > + kfree(xfer_data);
> > + return -EFAULT;
> > + }
> > +
> > + kfree(xfer_data);
> > + if (copy_to_user((void *)arg, &xfer, sizeof(struct jtag_xfer)))
> > + return -EFAULT;
> > + break;
> > +
> > + case JTAG_GIOCSTATUS:
> > + if (jtag->ops->status_get)
> > + err = jtag->ops->status_get(jtag, &value);
> > + else
> > + err = -EOPNOTSUPP;
> > + if (err)
> > + break;
> > +
> > + err = put_user(value, (__u32 *)arg);
>
> Same put_user err issue here.
Yes.
>
> > + break;
> > +
> > + default:
> > + return -EINVAL;
> > + }
> > + return err;
> > +}
> > +
> > +#ifdef CONFIG_COMPAT
> > +static long jtag_ioctl_compat(struct file *file, unsigned int cmd,
> > + unsigned long arg)
> > +{
> > + return jtag_ioctl(file, cmd, (unsigned long)compat_ptr(arg)); }
> > +#endif
>
> Why do you need a compat callback for a new ioctl? Create your structures
> properly and this should not be needed, right?
I does this because Arnd (arndbergmann@gmail.com) writed in review (On Wed, Aug 2, 2017 at 3:18 PM)
[..]
> + .unlocked_ioctl = jtag_ioctl,
> + .open = jtag_open,
> + .release = jtag_release,
> +};
add a compat_ioctl pointer here, after ensuring that all ioctl commands are compatible between 32-bit and 64-bit user space.
[..]
>
> > +
> > +static int jtag_open(struct inode *inode, struct file *file) {
> > + struct jtag *jtag = container_of(inode->i_cdev, struct jtag, cdev);
> > +
> > + if (atomic_read(&jtag->open)) {
> > + dev_info(NULL, "jtag already opened\n");
> > + return -EBUSY;
>
> Why do you care if multiple opens can happen?
Jtag HW not support to using with multiple requests from different users. So we prohibit this.
>
> > + }
> > +
> > + atomic_inc(&jtag->open);
>
> Oh look, you just raced with two different people opening at the same time :(
>
> An atomic value is never a replacement for a lock, sorry.
>
> > + file->private_data = jtag;
> > + return 0;
> > +}
> > +
> > +static int jtag_release(struct inode *inode, struct file *file) {
> > + struct jtag *jtag = file->private_data;
> > +
> > + atomic_dec(&jtag->open);
>
> Again, not needed.
>
> > + return 0;
> > +}
> > +
> > +static const struct file_operations jtag_fops = {
> > + .owner = THIS_MODULE,
> > + .open = jtag_open,
> > + .release = jtag_release,
> > + .llseek = noop_llseek,
> > + .unlocked_ioctl = jtag_ioctl,
> > +#ifdef CONFIG_COMPAT
> > + .compat_ioctl = jtag_ioctl_compat,
> > +#endif
> > +};
> > +
> > +struct jtag *jtag_alloc(size_t priv_size, const struct jtag_ops *ops)
> > +{
> > + struct jtag *jtag;
> > +
> > + jtag = kzalloc(sizeof(*jtag) + round_up(priv_size,
> ARCH_DMA_MINALIGN),
> > + GFP_KERNEL);
> > + if (!jtag)
> > + return NULL;
> > +
> > + jtag->ops = ops;
> > + return jtag;
> > +}
> > +EXPORT_SYMBOL_GPL(jtag_alloc);
>
> register should allocate and register the device, why pass a structure back that
> the caller then has to do something else with to use it?
>
Before registration we need to initialize JTAG device HW registers and fill private data with HW specific parameters.
And is I see in other Linux drivers it is a common way to separate allocation device and register it
> > +
> > +void jtag_free(struct jtag *jtag)
> > +{
> > + kfree(jtag);
> > +}
> > +EXPORT_SYMBOL_GPL(jtag_free);
> > +
> > +int jtag_register(struct jtag *jtag)
> > +{
> > + int id;
> > + int err;
> > +
> > + id = ida_simple_get(&jtag_ida, 0, 0, GFP_KERNEL);
> > + if (id < 0)
> > + return id;
> > +
> > + jtag->id = id;
> > + cdev_init(&jtag->cdev, &jtag_fops);
> > + jtag->cdev.owner = THIS_MODULE;
> > + err = cdev_add(&jtag->cdev, MKDEV(MAJOR(jtag_devt), jtag->id), 1);
> > + if (err)
> > + goto err_cdev;
>
> misc device would be so much simpler and easier here :(
>
>
> > +
> > + /* Register this jtag device with the driver core */
> > + jtag->dev = device_create(&jtag_class, NULL,
> MKDEV(MAJOR(jtag_devt),
> > + jtag->id),
> > + NULL, "jtag%d", jtag->id);
> > + if (!jtag->dev)
> > + goto err_device_create;
> > +
> > + atomic_set(&jtag->open, 0);
> > + dev_set_drvdata(jtag->dev, jtag);
> > + return err;
> > +
> > +err_device_create:
> > + cdev_del(&jtag->cdev);
> > +err_cdev:
> > + ida_simple_remove(&jtag_ida, id);
> > + return err;
> > +}
> > +EXPORT_SYMBOL_GPL(jtag_register);
> > +
> > +void jtag_unregister(struct jtag *jtag) {
> > + struct device *dev = jtag->dev;
> > +
> > + cdev_del(&jtag->cdev);
> > + device_unregister(dev);
> > + ida_simple_remove(&jtag_ida, jtag->id); }
> > +EXPORT_SYMBOL_GPL(jtag_unregister);
> > +
> > +static int __init jtag_init(void)
> > +{
> > + int err;
> > +
> > + err = alloc_chrdev_region(&jtag_devt, JTAG_FIRST_MINOR,
> > + JTAG_MAX_DEVICES, "jtag");
> > + if (err)
> > + return err;
> > +
> > + err = class_register(&jtag_class);
> > + if (err)
> > + unregister_chrdev_region(jtag_devt, JTAG_MAX_DEVICES);
> > +
> > + return err;
> > +}
> > +
> > +static void __exit jtag_exit(void)
> > +{
> > + class_unregister(&jtag_class);
> > + unregister_chrdev_region(jtag_devt, JTAG_MAX_DEVICES);
>
> Your idr leaked memory :(
>
Yes I will add ida_destroy() here.
>
>
> > +}
> > +
> > +module_init(jtag_init);
> > +module_exit(jtag_exit);
> > +
> > +MODULE_AUTHOR("Oleksandr Shamray <oleksandrs@mellanox.com>");
> > +MODULE_DESCRIPTION("Generic jtag support"); MODULE_LICENSE("GPL
> v2");
> > diff --git a/include/linux/jtag.h b/include/linux/jtag.h new file mode
> > 100644 index 0000000..c016fed
> > --- /dev/null
> > +++ b/include/linux/jtag.h
> > @@ -0,0 +1,48 @@
> > +/*
> > + * drivers/jtag/jtag.c
> > + *
> > + * Copyright (c) 2017 Mellanox Technologies. All rights reserved.
> > + * Copyright (c) 2017 Oleksandr Shamray <oleksandrs@mellanox.com>
> > + *
> > + * Released under the GPLv2 only.
> > + * SPDX-License-Identifier: GPL-2.0
> > + */
> > +
> > +#ifndef __JTAG_H
> > +#define __JTAG_H
> > +
> > +#include <uapi/linux/jtag.h>
> > +
> > +#ifndef ARCH_DMA_MINALIGN
> > +#define ARCH_DMA_MINALIGN 1
> > +#endif
> > +
> > +#define jtag_u64_to_ptr(arg) ((void *)(uintptr_t)arg)
> > +
> > +#define JTAG_MAX_XFER_DATA_LEN 65535
> > +
> > +struct jtag;
> > +/**
> > + * struct jtag_ops - callbacks for jtag control functions:
> > + *
> > + * @freq_get: get frequency function. Filled by device driver
> > + * @freq_set: set frequency function. Filled by device driver
> > + * @status_get: set status function. Filled by device driver
> > + * @idle: set JTAG to idle state function. Filled by device driver
> > + * @xfer: send JTAG xfer function. Filled by device driver */ struct
> > +jtag_ops {
> > + int (*freq_get)(struct jtag *jtag, u32 *freq);
> > + int (*freq_set)(struct jtag *jtag, u32 freq);
> > + int (*status_get)(struct jtag *jtag, u32 *state);
> > + int (*idle)(struct jtag *jtag, struct jtag_run_test_idle *idle);
> > + int (*xfer)(struct jtag *jtag, struct jtag_xfer *xfer); };
> > +
> > +void *jtag_priv(struct jtag *jtag);
> > +int jtag_register(struct jtag *jtag); void jtag_unregister(struct
> > +jtag *jtag); struct jtag *jtag_alloc(size_t priv_size, const struct
> > +jtag_ops *ops); void jtag_free(struct jtag *jtag);
> > +
> > +#endif /* __JTAG_H */
> > diff --git a/include/uapi/linux/jtag.h b/include/uapi/linux/jtag.h new
> > file mode 100644 index 0000000..180bf22
> > --- /dev/null
> > +++ b/include/uapi/linux/jtag.h
> > @@ -0,0 +1,115 @@
> > +/*
> > + * JTAG class driver
> > + *
> > + * Copyright (c) 2017 Mellanox Technologies. All rights reserved.
> > + * Copyright (c) 2017 Oleksandr Shamray <oleksandrs@mellanox.com>
> > + *
> > + * Released under the GPLv2/BSD.
> > + * SPDX-License-Identifier: GPL-2.0 OR BSD-3-Clause */
> > +
> > +#ifndef __UAPI_LINUX_JTAG_H
> > +#define __UAPI_LINUX_JTAG_H
> > +
> > +/**
> > + * enum jtag_xfer_mode:
> > + *
> > + * @JTAG_XFER_HW_MODE: hardware mode transfer
> > + * @JTAG_XFER_SW_MODE: software mode transfer */ enum
> jtag_xfer_mode
> > +{
> > + JTAG_XFER_HW_MODE,
> > + JTAG_XFER_SW_MODE,
> > +};
> > +
> > +/**
> > + * enum jtag_endstate:
> > + *
> > + * @JTAG_STATE_IDLE: JTAG state machine IDLE state
> > + * @JTAG_STATE_PAUSEIR: JTAG state machine PAUSE_IR state
> > + * @JTAG_STATE_PAUSEDR: JTAG state machine PAUSE_DR state */ enum
> > +jtag_endstate {
> > + JTAG_STATE_IDLE,
> > + JTAG_STATE_PAUSEIR,
> > + JTAG_STATE_PAUSEDR,
> > +};
> > +
> > +/**
> > + * enum jtag_xfer_type:
> > + *
> > + * @JTAG_SIR_XFER: SIR transfer
> > + * @JTAG_SDR_XFER: SDR transfer
> > + */
> > +enum jtag_xfer_type {
> > + JTAG_SIR_XFER,
> > + JTAG_SDR_XFER,
> > +};
> > +
> > +/**
> > + * enum jtag_xfer_direction:
> > + *
> > + * @JTAG_READ_XFER: read transfer
> > + * @JTAG_WRITE_XFER: write transfer
> > + */
> > +enum jtag_xfer_direction {
> > + JTAG_READ_XFER,
> > + JTAG_WRITE_XFER,
> > +};
> > +
> > +/**
> > + * struct jtag_run_test_idle - forces JTAG state machine to
> > + * RUN_TEST/IDLE state
> > + *
> > + * @mode: access mode
> > + * @reset: 0 - run IDLE/PAUSE from current state
> > + * 1 - go through TEST_LOGIC/RESET state before IDLE/PAUSE
> > + * @end: completion flag
> > + * @tck: clock counter
> > + *
> > + * Structure represents interface to JTAG device for jtag idle
> > + * execution.
> > + */
> > +struct jtag_run_test_idle {
> > + __u8 mode;
> > + __u8 reset;
> > + __u8 endstate;
> > + __u8 tck;
> > +};
> > +
> > +/**
> > + * struct jtag_xfer - jtag xfer:
> > + *
> > + * @mode: access mode
> > + * @type: transfer type
> > + * @direction: xfer direction
> > + * @length: xfer bits len
> > + * @tdio : xfer data array
> > + * @endir: xfer end state
> > + *
> > + * Structure represents interface to Aspeed JTAG device for jtag sdr
> > +xfer
> > + * execution.
> > + */
> > +struct jtag_xfer {
> > + __u8 mode;
> > + __u8 type;
> > + __u8 direction;
> > + __u8 endstate;
> > + __u32 length;
> > + __u64 tdio;
>
> I don't see anything odd here that requires a compat ioctl callback, do you?
>
> thanks,
>
> greg k-h
Thanks for your review.
Oleksandr S.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox