* Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function
From: Esben Haabendal @ 2019-05-14 12:41 UTC (permalink / raw)
To: Greg Kroah-Hartman
Cc: Lee Jones, linux-serial, Jiri Slaby, Nishanth Menon, Vignesh R,
Tony Lindgren, Lokesh Vutla, Florian Fainelli, linux-kernel
In-Reply-To: <20190514122618.GA18859@kroah.com>
Greg Kroah-Hartman <gregkh@linuxfoundation.org> writes:
> On Tue, May 14, 2019 at 11:47:41AM +0100, Lee Jones wrote:
>> On Tue, 14 May 2019, Esben Haabendal wrote:
>>
>> > Lee Jones <lee.jones@linaro.org> writes:
>> >
>> > > On Tue, 07 May 2019, Esben Haabendal wrote:
>> > >
>> > >> Lee Jones <lee.jones@linaro.org> writes:
>> > >>
>> > >> > On Fri, 26 Apr 2019, Esben Haabendal wrote:
>> > >> >
>> > >> >> The serial8250-mfd driver is for adding 8250/16550 UART ports as functions
>> > >> >> to an MFD driver.
>> > >> >>
>> > >> >> When calling mfd_add_device(), platform_data should be a pointer to a
>> > >> >> struct plat_serial8250_port, with proper settings like .flags, .type,
>> > >> >> .iotype, .regshift and .uartclk. Memory (or ioport) and IRQ should be
>> > >> >> passed as cell resources.
>> > >> >
>> > >> > What? No, please!
>> > >> >
>> > >> > If you *must* create a whole driver just to be able to use
>> > >> > platform_*() helpers (which I don't think you should), then please
>> > >> > call it something else. This doesn't have anything to do with MFD.
>> > >>
>> > >> True.
>> > >>
>> > >> I really don't think it is a good idea to create a whole driver just to
>> > >> be able to use platform_get_*() helpers. And if I am forced to do this,
>> > >> because I am unable to convince Andy to improve the standard serial8250
>> > >> driver to support that, it should be called MFD. The driver would be
>> > >
>> > > I assume you mean "shouldn't"?
>> >
>> > Of-course.
>> >
>> > >> generally usable for all usecases where platform_get_*() works.
>> > >>
>> > >> I don't have any idea what to call such a driver. It really would just
>> > >> be a fork of the current serial8250 driver, just allowing use of
>> > >> platform_get_*(), supporting exactly the same hardware.
>> > >>
>> > >> I am still hoping that we can find a way to improve serial8250 to be
>> > >> usable in these cases.
>> > >
>> > > Me too.
>> >
>> > Unfortunately, I don't seem to be able to convince Andy to accept
>> > something like that.
>>
>> Andy is not he Maintainer.
>>
>> What are Greg and Jiri's opinions?
>
> I've been ignoring all of this at the moment because of the 5.2-rc merge
> window. I'll look at it after -rc1 is out.
>
> thanks,
> greg k-h
Great, thanks!
I will try ad hold back with this thread until you get back to it.
/Esben
^ permalink raw reply
* Re: [PATCH v2 2/4] mfd: ioc3: Add driver for SGI IOC3 chip
From: Esben Haabendal @ 2019-05-14 12:39 UTC (permalink / raw)
To: Thomas Bogendoerfer
Cc: Lee Jones, Ralf Baechle, Paul Burton, James Hogan,
Dmitry Torokhov, David S. Miller, Alessandro Zummo,
Alexandre Belloni, Greg Kroah-Hartman, Jiri Slaby, linux-mips,
linux-kernel, linux-input, netdev, linux-rtc, linux-serial
In-Reply-To: <20190409154610.6735-3-tbogendoerfer@suse.de>
On Tue, 09 Apr 2019, Thomas Bogendoerfer wrote:
>
> diff --git a/drivers/net/ethernet/sgi/ioc3-eth.c b/drivers/net/ethernet/sgi/ioc3-eth.c
> index 358e66b81926..21fe722ebcd8 100644
> --- a/drivers/net/ethernet/sgi/ioc3-eth.c
> +++ b/drivers/net/ethernet/sgi/ioc3-eth.c
>
> [ ... ]
>
> - err = pci_request_regions(pdev, "ioc3");
Why are you dropping the call to pci_request_regions()? Shouldn't you
do something similar in the new mfd driver?
When you are use the the BAR 0 resource as mem_base argument to
mfd_add_devices() later on, it will be split into child resources for
the child devices, but they will not be related to the IORESOURCE_MEM
root tree (iomem_resource) anymore. I don't think that is how it is
supposed to be done, as it will allow random other drivers to request
the exact same memory area.
How/where is the memory resources inserted in the root IORESOURCE_MEM
resource (iomem_resource)? Or is it allowed to use resources without
inserting it into the root tree?
> + SET_NETDEV_DEV(dev, &pdev->dev);
> + ip = netdev_priv(dev);
> + r = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> + ip->regs = ioremap(r->start, resource_size(r));
> + r = platform_get_resource(pdev, IORESOURCE_MEM, 1);
> + ip->ssram = ioremap(r->start, resource_size(r));
Maybe use devm_platform_ioremap_resource() instead, which handles both
platform_get_resource() and ioremap() in one call..
/Esben
^ permalink raw reply
* Re: [PATCH] serial: 8250: Add support for using platform_device resources
From: Andy Shevchenko @ 2019-05-14 12:38 UTC (permalink / raw)
To: Esben Haabendal
Cc: Lee Jones, open list:SERIAL DRIVERS, Enrico Weigelt,
Greg Kroah-Hartman, Jiri Slaby, Darwin Dingel, Jisheng Zhang,
Sebastian Andrzej Siewior, He Zhe, Marek Vasut, Douglas Anderson,
Paul Burton, Linux Kernel Mailing List
In-Reply-To: <871s11meg3.fsf@haabendal.dk>
On Tue, May 14, 2019 at 02:02:36PM +0200, Esben Haabendal wrote:
> Andy Shevchenko <andy.shevchenko@gmail.com> writes:
> > On Tue, May 14, 2019 at 10:24 AM Esben Haabendal <esben@haabendal.dk> wrote:
> >> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> >> > On Tue, May 07, 2019 at 02:22:18PM +0200, Esben Haabendal wrote:
> >
> >> We are on repeat here. I don't agree with you here. I have a simple
> >> generic 8250 (16550A) compatible device, and cannot use it in a mfd
> >> driver using the standard mfd-core framework.
> >
> >> The lacking of support for platform_get_resource() in the generic
> >> serial8250 driver is not a feature. It should be supported, just as it
> >> is in several of the specialized 8250 drivers.
> >
> > We are going circles here.
> > What exactly prevents you to use it? Presence of request_mem_region()?
>
> Exactly.
And I completely tired to repeat that this is okay and does not prevent you to
use MFD.
> >> It would still mean that I would have revert to not using convenient and
> >> otherwise fully appropriate API calls like pci_request_regions() and
> >> mfd_add_devices().
> >
> > Yes, here is the issue. 8250 requires the parent not to *request*
> > resources. Because child handles IO access itself.
>
> Ok, clearly we are not discussing the actual IO access. The only issue
> is how to handle the memory resource management.
>
> And yes, serial8250 requires "the parent" to not request the resources.
> But by doing so, it gets in the way of the mfd-core way of splitting a
> properly requested resource.
Which is right thing to do. Requesting resources should be done by their actual
user, no?
Moreover, if you look at /proc/iomem, for example, I bet there will be
a difference with your proposed method and a established one.
> >> The mfd driver in question is for a PCI device. Not being able to
> >> request the PCI regions seems silly.
> >
> > Nope. Otherwise, the parent which *doesn't handle* IO on behalf of
> > child should not request its resources.
>
> If I may, could I get you to discuss this with Lee Jones?
>
> As I read both of your comments in this thread, you are not aligned on
> how mfd drivers should handle resources. And in that case, one of you
> are most likely more right than the other, and if Lee is right, I seem
> to be unable to convince you about that.
MFD has nothing to do with *requesting* resource. It's about *slicing* them.
*Requesting* resource is orthogonal to the *slicing*.
> >> Not being able to register all child devices with the call introduced
> >> for that sole purpose also seems silly.
> >
> >> Please take a look at https://lkml.org/lkml/2019/4/9/576
> >> ("[PATCH v2 2/4] mfd: ioc3: Add driver for SGI IOC3 chip")
> >
> > Thank you for this link.
> > Now, look at this comment:
> >
> > + /*
> > + * Map all IOC3 registers. These are shared between subdevices
> > + * so the main IOC3 module manages them.
> > + */
> >
> > Is it your case? Can we see the code?
>
> That comment seems quite misleading. I am quote certain that the uart
> registers which are part of BAR 0 is not more shared between subdevices
> than the uart registers in BAR 0 in my case.
>
> But BAR 0 as a whole is shared between subdevices. But BAR 0 can be
> (is) split in parts that are exclusive to one subdevice. The only
> difference I see is that I don't have any registers accessed directly by
> the mfd driver.
I stopped here. Let's discuss an actual code.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function
From: Greg Kroah-Hartman @ 2019-05-14 12:26 UTC (permalink / raw)
To: Lee Jones
Cc: Esben Haabendal, linux-serial, Jiri Slaby, Nishanth Menon,
Vignesh R, Tony Lindgren, Lokesh Vutla, Florian Fainelli,
linux-kernel
In-Reply-To: <20190514104741.GO4319@dell>
On Tue, May 14, 2019 at 11:47:41AM +0100, Lee Jones wrote:
> On Tue, 14 May 2019, Esben Haabendal wrote:
>
> > Lee Jones <lee.jones@linaro.org> writes:
> >
> > > On Tue, 07 May 2019, Esben Haabendal wrote:
> > >
> > >> Lee Jones <lee.jones@linaro.org> writes:
> > >>
> > >> > On Fri, 26 Apr 2019, Esben Haabendal wrote:
> > >> >
> > >> >> The serial8250-mfd driver is for adding 8250/16550 UART ports as functions
> > >> >> to an MFD driver.
> > >> >>
> > >> >> When calling mfd_add_device(), platform_data should be a pointer to a
> > >> >> struct plat_serial8250_port, with proper settings like .flags, .type,
> > >> >> .iotype, .regshift and .uartclk. Memory (or ioport) and IRQ should be
> > >> >> passed as cell resources.
> > >> >
> > >> > What? No, please!
> > >> >
> > >> > If you *must* create a whole driver just to be able to use
> > >> > platform_*() helpers (which I don't think you should), then please
> > >> > call it something else. This doesn't have anything to do with MFD.
> > >>
> > >> True.
> > >>
> > >> I really don't think it is a good idea to create a whole driver just to
> > >> be able to use platform_get_*() helpers. And if I am forced to do this,
> > >> because I am unable to convince Andy to improve the standard serial8250
> > >> driver to support that, it should be called MFD. The driver would be
> > >
> > > I assume you mean "shouldn't"?
> >
> > Of-course.
> >
> > >> generally usable for all usecases where platform_get_*() works.
> > >>
> > >> I don't have any idea what to call such a driver. It really would just
> > >> be a fork of the current serial8250 driver, just allowing use of
> > >> platform_get_*(), supporting exactly the same hardware.
> > >>
> > >> I am still hoping that we can find a way to improve serial8250 to be
> > >> usable in these cases.
> > >
> > > Me too.
> >
> > Unfortunately, I don't seem to be able to convince Andy to accept
> > something like that.
>
> Andy is not he Maintainer.
>
> What are Greg and Jiri's opinions?
I've been ignoring all of this at the moment because of the 5.2-rc merge
window. I'll look at it after -rc1 is out.
thanks,
greg k-h
^ permalink raw reply
* Re: [PATCH] serial: 8250: Add support for using platform_device resources
From: Esben Haabendal @ 2019-05-14 12:02 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andy Shevchenko, Lee Jones, open list:SERIAL DRIVERS,
Enrico Weigelt, Greg Kroah-Hartman, Jiri Slaby, Darwin Dingel,
Jisheng Zhang, Sebastian Andrzej Siewior, He Zhe, Marek Vasut,
Douglas Anderson, Paul Burton, Linux Kernel Mailing List
In-Reply-To: <CAHp75VetoajaeqUnUuj4sNjhujqDkbqvQmxE+LMtzFN4so_jwA@mail.gmail.com>
Andy Shevchenko <andy.shevchenko@gmail.com> writes:
> On Tue, May 14, 2019 at 12:23 PM Andy Shevchenko
> <andy.shevchenko@gmail.com> wrote:
>> On Tue, May 14, 2019 at 10:24 AM Esben Haabendal <esben@haabendal.dk> wrote:
>
>> > Please take a look at https://lkml.org/lkml/2019/4/9/576
>> > ("[PATCH v2 2/4] mfd: ioc3: Add driver for SGI IOC3 chip")
>>
>> Thank you for this link.
>> Now, look at this comment:
>>
>> + /*
>> + * Map all IOC3 registers. These are shared between subdevices
>> + * so the main IOC3 module manages them.
>> + */
>>
>> Is it your case? Can we see the code?
>
> They do not request resources by the way.
Actually, that looks like a bug in ioc3.c driver.
It is using mfd_add_devices() with a mem_base that has not been properly
requested, and the platform_get_resource() calls made by child drivers
does not guarantee exclusive access to the memory resources, as they are
not inserted in the root memory resource tree.
> You may do the same, I told you this several times.
In drivers/mfd/ioc3.c:
First, the uart resources are defined. The register memory resource is
defined relative to the mfd driver memory resource.
+static struct resource ioc3_uarta_resources[] = {
+ DEFINE_RES_MEM(offsetof(struct ioc3, sregs.uarta),
+ sizeof_field(struct ioc3, sregs.uarta)),
+ DEFINE_RES_IRQ(6)
+};
This is then used when creating the uart cell.
+ cell->name = "ioc3-serial8250";
+ cell->id = ioc3_serial_id++;
+ cell->resources = ioc3_uarta_resources;
+ cell->num_resources = ARRAY_SIZE(ioc3_uarta_resources);
Finally, the mfd_add_devices() call is made, giving the resource for the
BAR0 region (&ipd->pdev->resource[0]) as mem_base argument:
+ mfd_add_devices(&ipd->pdev->dev, -1, ioc3_mfd_cells,
+ cell - ioc3_mfd_cells, &ipd->pdev->resource[0],
+ 0, ipd->domain);
This is just what I want to do.
But in order to guarantee exclusive access to the memory resource, I
need to have it requested.
/Esben
^ permalink raw reply
* Re: [PATCH] serial: 8250: Add support for using platform_device resources
From: Esben Haabendal @ 2019-05-14 12:02 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Andy Shevchenko, Lee Jones, open list:SERIAL DRIVERS,
Enrico Weigelt, Greg Kroah-Hartman, Jiri Slaby, Darwin Dingel,
Jisheng Zhang, Sebastian Andrzej Siewior, He Zhe, Marek Vasut,
Douglas Anderson, Paul Burton, Linux Kernel Mailing List
In-Reply-To: <CAHp75VfrP6SLVzmp6LepN7dU1c7QYxfRDRtj7dCTuWzmYp2tCA@mail.gmail.com>
Andy Shevchenko <andy.shevchenko@gmail.com> writes:
> On Tue, May 14, 2019 at 10:24 AM Esben Haabendal <esben@haabendal.dk> wrote:
>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>> > On Tue, May 07, 2019 at 02:22:18PM +0200, Esben Haabendal wrote:
>
>> We are on repeat here. I don't agree with you here. I have a simple
>> generic 8250 (16550A) compatible device, and cannot use it in a mfd
>> driver using the standard mfd-core framework.
>
>> The lacking of support for platform_get_resource() in the generic
>> serial8250 driver is not a feature. It should be supported, just as it
>> is in several of the specialized 8250 drivers.
>
> We are going circles here.
> What exactly prevents you to use it? Presence of request_mem_region()?
Exactly.
>> It would still mean that I would have revert to not using convenient and
>> otherwise fully appropriate API calls like pci_request_regions() and
>> mfd_add_devices().
>
> Yes, here is the issue. 8250 requires the parent not to *request*
> resources. Because child handles IO access itself.
Ok, clearly we are not discussing the actual IO access. The only issue
is how to handle the memory resource management.
And yes, serial8250 requires "the parent" to not request the resources.
But by doing so, it gets in the way of the mfd-core way of splitting a
properly requested resource.
>> The mfd driver in question is for a PCI device. Not being able to
>> request the PCI regions seems silly.
>
> Nope. Otherwise, the parent which *doesn't handle* IO on behalf of
> child should not request its resources.
If I may, could I get you to discuss this with Lee Jones?
As I read both of your comments in this thread, you are not aligned on
how mfd drivers should handle resources. And in that case, one of you
are most likely more right than the other, and if Lee is right, I seem
to be unable to convince you about that.
>> Not being able to register all child devices with the call introduced
>> for that sole purpose also seems silly.
>
>> Please take a look at https://lkml.org/lkml/2019/4/9/576
>> ("[PATCH v2 2/4] mfd: ioc3: Add driver for SGI IOC3 chip")
>
> Thank you for this link.
> Now, look at this comment:
>
> + /*
> + * Map all IOC3 registers. These are shared between subdevices
> + * so the main IOC3 module manages them.
> + */
>
> Is it your case? Can we see the code?
That comment seems quite misleading. I am quote certain that the uart
registers which are part of BAR 0 is not more shared between subdevices
than the uart registers in BAR 0 in my case.
But BAR 0 as a whole is shared between subdevices. But BAR 0 can be
(is) split in parts that are exclusive to one subdevice. The only
difference I see is that I don't have any registers accessed directly by
the mfd driver.
/Esben
^ permalink raw reply
* Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function
From: Lee Jones @ 2019-05-14 10:47 UTC (permalink / raw)
To: Esben Haabendal
Cc: linux-serial, Greg Kroah-Hartman, Jiri Slaby, Nishanth Menon,
Vignesh R, Tony Lindgren, Lokesh Vutla, Florian Fainelli,
linux-kernel
In-Reply-To: <87bm05mpmx.fsf@haabendal.dk>
On Tue, 14 May 2019, Esben Haabendal wrote:
> Lee Jones <lee.jones@linaro.org> writes:
>
> > On Tue, 07 May 2019, Esben Haabendal wrote:
> >
> >> Lee Jones <lee.jones@linaro.org> writes:
> >>
> >> > On Fri, 26 Apr 2019, Esben Haabendal wrote:
> >> >
> >> >> The serial8250-mfd driver is for adding 8250/16550 UART ports as functions
> >> >> to an MFD driver.
> >> >>
> >> >> When calling mfd_add_device(), platform_data should be a pointer to a
> >> >> struct plat_serial8250_port, with proper settings like .flags, .type,
> >> >> .iotype, .regshift and .uartclk. Memory (or ioport) and IRQ should be
> >> >> passed as cell resources.
> >> >
> >> > What? No, please!
> >> >
> >> > If you *must* create a whole driver just to be able to use
> >> > platform_*() helpers (which I don't think you should), then please
> >> > call it something else. This doesn't have anything to do with MFD.
> >>
> >> True.
> >>
> >> I really don't think it is a good idea to create a whole driver just to
> >> be able to use platform_get_*() helpers. And if I am forced to do this,
> >> because I am unable to convince Andy to improve the standard serial8250
> >> driver to support that, it should be called MFD. The driver would be
> >
> > I assume you mean "shouldn't"?
>
> Of-course.
>
> >> generally usable for all usecases where platform_get_*() works.
> >>
> >> I don't have any idea what to call such a driver. It really would just
> >> be a fork of the current serial8250 driver, just allowing use of
> >> platform_get_*(), supporting exactly the same hardware.
> >>
> >> I am still hoping that we can find a way to improve serial8250 to be
> >> usable in these cases.
> >
> > Me too.
>
> Unfortunately, I don't seem to be able to convince Andy to accept
> something like that.
Andy is not he Maintainer.
What are Greg and Jiri's opinions?
> I might have to do this out-of-tree :(
Well that would suck!
--
Lee Jones [李琼斯]
Linaro Services Technical Lead
Linaro.org │ Open source software for ARM SoCs
Follow Linaro: Facebook | Twitter | Blog
^ permalink raw reply
* [PATCH 7/7] tty: max310x: Split uart characters insertion loop
From: Serge Semin @ 2019-05-14 10:14 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby; +Cc: Serge Semin, linux-serial, linux-kernel
In-Reply-To: <20190514101415.26754-1-fancer.lancer@gmail.com>
Batch read mode doesn't check any conditions or flags except the Rx
overflow one. But it may only happen after the last character is pushed
into the RHR register. In this case we shouldn't push all the read
characters with overrun flag set, but only the last one caused the
FIFO overflow. This commit splits the characters retrieval loop into
two parts. First one is ordinary intsert-chars procedure without taking
the overrun status into account. Second part inserts the last character
checking whether the overrun happened and pushing a '\0' character with
TTY_OVERRUN flag to a flip-buffer.
If we left the loop the way it was the '\0' character would be inserted
after each character retrieved at the overrun occasion.
Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
drivers/tty/serial/max310x.c | 14 ++++++++++----
1 file changed, 10 insertions(+), 4 deletions(-)
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 36943f6c198c..81b2413c3da4 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -680,10 +680,16 @@ static void max310x_handle_rx(struct uart_port *port, unsigned int rxlen)
port->icount.overrun++;
}
- for (i = 0; i < rxlen; ++i) {
- uart_insert_char(port, sts, MAX310X_LSR_RXOVR_BIT,
- one->rx_buf[i], flag);
- }
+ for (i = 0; i < (rxlen - 1); ++i)
+ uart_insert_char(port, sts, 0, one->rx_buf[i], flag);
+
+ /*
+ * Handle the overrun case for the last character only, since
+ * the RxFIFO overflow happens after it is pushed to the FIFO
+ * tail.
+ */
+ uart_insert_char(port, sts, MAX310X_LSR_RXOVR_BIT,
+ one->rx_buf[rxlen], flag);
} else {
if (unlikely(rxlen >= port->fifosize)) {
--
2.21.0
^ permalink raw reply related
* [PATCH 6/7] tty: max310x: Optionally enable rs485 on startup
From: Serge Semin @ 2019-05-14 10:14 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby; +Cc: Serge Semin, linux-serial, linux-kernel
In-Reply-To: <20190514101415.26754-1-fancer.lancer@gmail.com>
UART port might be pre-configured with rs485 enabled flag at the
time of the port starting up process. In this case we need to
have the hardware rs485-related registers initialized in accordance
with the rs485 flags and settings provided by the configs descriptor.
Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
drivers/tty/serial/max310x.c | 16 ++++++++++++++++
1 file changed, 16 insertions(+)
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 2255300404bd..36943f6c198c 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -1030,6 +1030,22 @@ static int max310x_startup(struct uart_port *port)
max310x_port_update(port, MAX310X_MODE2_REG,
MAX310X_MODE2_FIFORST_BIT, 0);
+ /* Configure mode1/mode2 to have rs485/rs232 enabled at startup */
+ val = (clamp(port->rs485.delay_rts_before_send, 0U, 15U) << 4) |
+ clamp(port->rs485.delay_rts_after_send, 0U, 15U);
+ max310x_port_write(port, MAX310X_HDPIXDELAY_REG, val);
+
+ if (port->rs485.flags & SER_RS485_ENABLED) {
+ max310x_port_update(port, MAX310X_MODE1_REG,
+ MAX310X_MODE1_TRNSCVCTRL_BIT,
+ MAX310X_MODE1_TRNSCVCTRL_BIT);
+
+ if (!(port->rs485.flags & SER_RS485_RX_DURING_TX))
+ max310x_port_update(port, MAX310X_MODE2_REG,
+ MAX310X_MODE2_ECHOSUPR_BIT,
+ MAX310X_MODE2_ECHOSUPR_BIT);
+ }
+
/* Configure flow control levels */
/* Flow control halt level 96, resume level 48 */
max310x_port_write(port, MAX310X_FLOWLVL_REG,
--
2.21.0
^ permalink raw reply related
* [PATCH 5/7] tty: max310x: Add rx-during-tx rs485 flag support
From: Serge Semin @ 2019-05-14 10:14 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby; +Cc: Serge Semin, linux-serial, linux-kernel
In-Reply-To: <20190514101415.26754-1-fancer.lancer@gmail.com>
The driver currently sets the echo suppression bit by default when rs485
is enabled. Naturally it disables any data retrieval in rs485 mode while
RTSn is pushed up. The receiver gate (RX_) can be enabled just by clearing
(or not setting) the EchoSuprs bit of mode2 register. So by setting or
clearing the bit we implement the SER_RS485_RX_DURING_TX rs485 flag
support.
Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
drivers/tty/serial/max310x.c | 29 ++++++++++++++---------------
1 file changed, 14 insertions(+), 15 deletions(-)
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index ca044f96c5cc..2255300404bd 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -975,25 +975,23 @@ static void max310x_set_termios(struct uart_port *port,
static void max310x_rs_proc(struct work_struct *ws)
{
struct max310x_one *one = container_of(ws, struct max310x_one, rs_work);
- unsigned int val;
+ unsigned int delay, mode1 = 0, mode2 = 0;
- val = (one->port.rs485.delay_rts_before_send << 4) |
+ delay = (one->port.rs485.delay_rts_before_send << 4) |
one->port.rs485.delay_rts_after_send;
- max310x_port_write(&one->port, MAX310X_HDPIXDELAY_REG, val);
+ max310x_port_write(&one->port, MAX310X_HDPIXDELAY_REG, delay);
if (one->port.rs485.flags & SER_RS485_ENABLED) {
- max310x_port_update(&one->port, MAX310X_MODE1_REG,
- MAX310X_MODE1_TRNSCVCTRL_BIT,
- MAX310X_MODE1_TRNSCVCTRL_BIT);
- max310x_port_update(&one->port, MAX310X_MODE2_REG,
- MAX310X_MODE2_ECHOSUPR_BIT,
- MAX310X_MODE2_ECHOSUPR_BIT);
- } else {
- max310x_port_update(&one->port, MAX310X_MODE1_REG,
- MAX310X_MODE1_TRNSCVCTRL_BIT, 0);
- max310x_port_update(&one->port, MAX310X_MODE2_REG,
- MAX310X_MODE2_ECHOSUPR_BIT, 0);
+ mode1 = MAX310X_MODE1_TRNSCVCTRL_BIT;
+
+ if (!(one->port.rs485.flags & SER_RS485_RX_DURING_TX))
+ mode2 = MAX310X_MODE2_ECHOSUPR_BIT;
}
+
+ max310x_port_update(&one->port, MAX310X_MODE1_REG,
+ MAX310X_MODE1_TRNSCVCTRL_BIT, mode1);
+ max310x_port_update(&one->port, MAX310X_MODE2_REG,
+ MAX310X_MODE2_ECHOSUPR_BIT, mode2);
}
static int max310x_rs485_config(struct uart_port *port,
@@ -1005,7 +1003,8 @@ static int max310x_rs485_config(struct uart_port *port,
(rs485->delay_rts_after_send > 0x0f))
return -ERANGE;
- rs485->flags &= SER_RS485_RTS_ON_SEND | SER_RS485_ENABLED;
+ rs485->flags &= SER_RS485_RTS_ON_SEND | SER_RS485_RX_DURING_TX |
+ SER_RS485_ENABLED;
memset(rs485->padding, 0, sizeof(rs485->padding));
port->rs485 = *rs485;
--
2.21.0
^ permalink raw reply related
* [PATCH 4/7] tty: max310x: Fix invalid baudrate divisors calculator
From: Serge Semin @ 2019-05-14 10:14 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby; +Cc: Serge Semin, linux-serial, linux-kernel
In-Reply-To: <20190514101415.26754-1-fancer.lancer@gmail.com>
Current calculator doesn't do it' job quite correct. First of all the
max310x baud-rates generator supports the divisor being less than 16.
In this case the x2/x4 modes can be used to double or quadruple
the reference frequency. But the current baud-rate setter function
just filters all these modes out by the first condition and setups
these modes only if there is a clocks-baud division remainder. The former
doesn't seem right at all, since enabling the x2/x4 modes causes the line
noise tolerance reduction and should be only used as a last resort to
enable a requested too high baud-rate.
Finally the fraction is supposed to be calculated from D = Fref/(c*baud)
formulae, but not from D % 16, which causes the precision loss. So to speak
the current baud-rate calculator code works well only if the baud perfectly
fits to the uart reference input frequency.
Lets fix the calculator by implementing the algo fully compliant with
the fractional baud-rate generator described in the datasheet:
D = Fref / (c*baud), where c={16,8,4} is the x1/x2/x4 rate mode
respectively, Fref - reference input frequency. The divisor fraction is
calculated from the same formulae, but making sure it is found with a
resolution of 0.0625 (four bits).
Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
drivers/tty/serial/max310x.c | 51 ++++++++++++++++++++++--------------
1 file changed, 31 insertions(+), 20 deletions(-)
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 0e3dc89c459b..ca044f96c5cc 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -501,37 +501,48 @@ static bool max310x_reg_precious(struct device *dev, unsigned int reg)
static int max310x_set_baud(struct uart_port *port, int baud)
{
- unsigned int mode = 0, clk = port->uartclk, div = clk / baud;
+ unsigned int mode = 0, div = 0, frac = 0, c = 0, F = 0;
- /* Check for minimal value for divider */
- if (div < 16)
- div = 16;
-
- if (clk % baud && (div / 16) < 0x8000) {
+ /*
+ * Calculate the integer divisor first. Select a proper mode
+ * in case if the requested baud is too high for the pre-defined
+ * clocks frequency.
+ */
+ div = port->uartclk / baud;
+ if (div < 8) {
+ /* Mode x4 */
+ c = 4;
+ mode = MAX310X_BRGCFG_4XMODE_BIT;
+ } else if (div < 16) {
/* Mode x2 */
+ c = 8;
mode = MAX310X_BRGCFG_2XMODE_BIT;
- clk = port->uartclk * 2;
- div = clk / baud;
-
- if (clk % baud && (div / 16) < 0x8000) {
- /* Mode x4 */
- mode = MAX310X_BRGCFG_4XMODE_BIT;
- clk = port->uartclk * 4;
- div = clk / baud;
- }
+ } else {
+ c = 16;
}
- max310x_port_write(port, MAX310X_BRGDIVMSB_REG, (div / 16) >> 8);
- max310x_port_write(port, MAX310X_BRGDIVLSB_REG, div / 16);
- max310x_port_write(port, MAX310X_BRGCFG_REG, (div % 16) | mode);
+ /* Calculate the divisor in accordance with the fraction coefficient */
+ div /= c;
+ F = c*baud;
+
+ /* Calculate the baud rate fraction */
+ if (div > 0)
+ frac = (16*(port->uartclk % F)) / F;
+ else
+ div = 1;
+
+ max310x_port_write(port, MAX310X_BRGDIVMSB_REG, div >> 8);
+ max310x_port_write(port, MAX310X_BRGDIVLSB_REG, div);
+ max310x_port_write(port, MAX310X_BRGCFG_REG, frac | mode);
- return DIV_ROUND_CLOSEST(clk, div);
+ /* Return the actual baud rate we just programmed */
+ return (16*port->uartclk) / (c*(16*div + frac));
}
static int max310x_update_best_err(unsigned long f, long *besterr)
{
/* Use baudrate 115200 for calculate error */
- long err = f % (115200 * 16);
+ long err = f % (460800 * 16);
if ((*besterr < 0) || (*besterr > err)) {
*besterr = err;
--
2.21.0
^ permalink raw reply related
* [PATCH 3/7] tty: max310x: Don't pass stacked buffers to SPI
From: Serge Semin @ 2019-05-14 10:14 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby; +Cc: Serge Semin, linux-serial, linux-kernel
In-Reply-To: <20190514101415.26754-1-fancer.lancer@gmail.com>
SPI transfer tx/rx buffers must be DMA-safe and the structure
documentation clearly states this. Data declared on the system stack isn't
DMA-safe [1]. Instead at least kernel memory should be used for the
buffers. In order to fix this here we can create the buffers at the device
probing stage and use them without any synchronization, since batch
read/write methods are called from non-reentrant contexts - either from
rx-event IRQ threaded handler or from the tx workqueue item.
[1] Documentation/DMA-API-HOWTO.txt
Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
drivers/tty/serial/max310x.c | 27 ++++++++++++++++++---------
1 file changed, 18 insertions(+), 9 deletions(-)
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 527f1476c24a..0e3dc89c459b 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -258,6 +258,10 @@ struct max310x_one {
struct work_struct tx_work;
struct work_struct md_work;
struct work_struct rs_work;
+
+ u8 wr_header;
+ u8 rd_header;
+ u8 rx_buf[MAX310X_FIFO_SIZE];
};
#define to_max310x_port(_port) \
container_of(_port, struct max310x_one, port)
@@ -608,11 +612,11 @@ static int max310x_set_ref_clk(struct device *dev, struct max310x_port *s,
static void max310x_batch_write(struct uart_port *port, u8 *txbuf, unsigned int len)
{
- u8 header[] = { (port->iobase + MAX310X_THR_REG) | MAX310X_WRITE_BIT };
+ struct max310x_one *one = to_max310x_port(port);
struct spi_transfer xfer[] = {
{
- .tx_buf = &header,
- .len = sizeof(header),
+ .tx_buf = &one->wr_header,
+ .len = sizeof(one->wr_header),
}, {
.tx_buf = txbuf,
.len = len,
@@ -623,11 +627,11 @@ static void max310x_batch_write(struct uart_port *port, u8 *txbuf, unsigned int
static void max310x_batch_read(struct uart_port *port, u8 *rxbuf, unsigned int len)
{
- u8 header[] = { port->iobase + MAX310X_RHR_REG };
+ struct max310x_one *one = to_max310x_port(port);
struct spi_transfer xfer[] = {
{
- .tx_buf = &header,
- .len = sizeof(header),
+ .tx_buf = &one->rd_header,
+ .len = sizeof(one->rd_header),
}, {
.rx_buf = rxbuf,
.len = len,
@@ -638,8 +642,8 @@ static void max310x_batch_read(struct uart_port *port, u8 *rxbuf, unsigned int l
static void max310x_handle_rx(struct uart_port *port, unsigned int rxlen)
{
+ struct max310x_one *one = to_max310x_port(port);
unsigned int sts, ch, flag, i;
- u8 buf[MAX310X_FIFO_SIZE];
if (port->read_status_mask == MAX310X_LSR_RXOVR_BIT) {
/* We are just reading, happily ignoring any error conditions.
@@ -654,7 +658,7 @@ static void max310x_handle_rx(struct uart_port *port, unsigned int rxlen)
* */
sts = max310x_port_read(port, MAX310X_LSR_IRQSTS_REG);
- max310x_batch_read(port, buf, rxlen);
+ max310x_batch_read(port, one->rx_buf, rxlen);
port->icount.rx += rxlen;
flag = TTY_NORMAL;
@@ -666,7 +670,8 @@ static void max310x_handle_rx(struct uart_port *port, unsigned int rxlen)
}
for (i = 0; i < rxlen; ++i) {
- uart_insert_char(port, sts, MAX310X_LSR_RXOVR_BIT, buf[i], flag);
+ uart_insert_char(port, sts, MAX310X_LSR_RXOVR_BIT,
+ one->rx_buf[i], flag);
}
} else {
@@ -1298,6 +1303,10 @@ static int max310x_probe(struct device *dev, struct max310x_devtype *devtype,
INIT_WORK(&s->p[i].md_work, max310x_md_proc);
/* Initialize queue for changing RS485 mode */
INIT_WORK(&s->p[i].rs_work, max310x_rs_proc);
+ /* Initialize SPI-transfer buffers */
+ s->p[i].wr_header = (s->p[i].port.iobase + MAX310X_THR_REG) |
+ MAX310X_WRITE_BIT;
+ s->p[i].rd_header = (s->p[i].port.iobase + MAX310X_RHR_REG);
/* Register port */
ret = uart_add_one_port(&max310x_uart, &s->p[i].port);
--
2.21.0
^ permalink raw reply related
* [PATCH 2/7] tty: max310x: Introduce max310x_one port macro-wrapper
From: Serge Semin @ 2019-05-14 10:14 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby; +Cc: Serge Semin, linux-serial, linux-kernel
In-Reply-To: <20190514101415.26754-1-fancer.lancer@gmail.com>
uart_port structure instance is embedded into the max310x_one
super-structure, which is accessed by some of the uart-port callback
methods. In order to improve the callback's code readability lets
define the to_max310x_port() wrapper which just translates the passed
uart_port pointer to the max310x_one one. It is also going to be
handy in future commits.
Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
drivers/tty/serial/max310x.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 4ee805862b68..527f1476c24a 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -259,6 +259,8 @@ struct max310x_one {
struct work_struct md_work;
struct work_struct rs_work;
};
+#define to_max310x_port(_port) \
+ container_of(_port, struct max310x_one, port)
struct max310x_port {
struct max310x_devtype *devtype;
@@ -765,7 +767,7 @@ static void max310x_handle_tx(struct uart_port *port)
static void max310x_start_tx(struct uart_port *port)
{
- struct max310x_one *one = container_of(port, struct max310x_one, port);
+ struct max310x_one *one = to_max310x_port(port);
schedule_work(&one->tx_work);
}
@@ -858,7 +860,7 @@ static void max310x_md_proc(struct work_struct *ws)
static void max310x_set_mctrl(struct uart_port *port, unsigned int mctrl)
{
- struct max310x_one *one = container_of(port, struct max310x_one, port);
+ struct max310x_one *one = to_max310x_port(port);
schedule_work(&one->md_work);
}
@@ -981,7 +983,7 @@ static void max310x_rs_proc(struct work_struct *ws)
static int max310x_rs485_config(struct uart_port *port,
struct serial_rs485 *rs485)
{
- struct max310x_one *one = container_of(port, struct max310x_one, port);
+ struct max310x_one *one = to_max310x_port(port);
if ((rs485->delay_rts_before_send > 0x0f) ||
(rs485->delay_rts_after_send > 0x0f))
--
2.21.0
^ permalink raw reply related
* [PATCH 1/7] tty: max310x: Simplify tx-work item code
From: Serge Semin @ 2019-05-14 10:14 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby; +Cc: Serge Semin, linux-serial, linux-kernel
In-Reply-To: <20190514101415.26754-1-fancer.lancer@gmail.com>
Since cmwq introduction in the kernel, workqueues've been turned into
non-reentrant execution contexts [1]. It means any work item is
guaranteed to be executed by at most one worker system-wide at any
given time. Since tx-handler max310x_handle_tx() is called by a
single work item we don't need it to be self-protected by the mutex.
We also don't need to check the tx work item pending state before
scheduling it (which in the first place was racy btw), since cmwq will
make sure to reschedule the item if it wasn't pending at the moment of
schedule_work() call.
[1] Documentation/core-api/workqueue.rst
Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
---
drivers/tty/serial/max310x.c | 16 +++-------------
1 file changed, 3 insertions(+), 13 deletions(-)
diff --git a/drivers/tty/serial/max310x.c b/drivers/tty/serial/max310x.c
index 450ba6d7996c..4ee805862b68 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -263,7 +263,6 @@ struct max310x_one {
struct max310x_port {
struct max310x_devtype *devtype;
struct regmap *regmap;
- struct mutex mutex;
struct clk *clk;
#ifdef CONFIG_GPIOLIB
struct gpio_chip gpio;
@@ -768,8 +767,7 @@ static void max310x_start_tx(struct uart_port *port)
{
struct max310x_one *one = container_of(port, struct max310x_one, port);
- if (!work_pending(&one->tx_work))
- schedule_work(&one->tx_work);
+ schedule_work(&one->tx_work);
}
static irqreturn_t max310x_port_irq(struct max310x_port *s, int portno)
@@ -826,14 +824,11 @@ static irqreturn_t max310x_ist(int irq, void *dev_id)
return IRQ_RETVAL(handled);
}
-static void max310x_wq_proc(struct work_struct *ws)
+static void max310x_tx_proc(struct work_struct *ws)
{
struct max310x_one *one = container_of(ws, struct max310x_one, tx_work);
- struct max310x_port *s = dev_get_drvdata(one->port.dev);
- mutex_lock(&s->mutex);
max310x_handle_tx(&one->port);
- mutex_unlock(&s->mutex);
}
static unsigned int max310x_tx_empty(struct uart_port *port)
@@ -1269,8 +1264,6 @@ static int max310x_probe(struct device *dev, struct max310x_devtype *devtype,
uartclk = max310x_set_ref_clk(dev, s, freq, xtal);
dev_dbg(dev, "Reference clock set to %i Hz\n", uartclk);
- mutex_init(&s->mutex);
-
for (i = 0; i < devtype->nr; i++) {
unsigned int line;
@@ -1298,7 +1291,7 @@ static int max310x_probe(struct device *dev, struct max310x_devtype *devtype,
/* Clear IRQ status register */
max310x_port_read(&s->p[i].port, MAX310X_IRQSTS_REG);
/* Initialize queue for start TX */
- INIT_WORK(&s->p[i].tx_work, max310x_wq_proc);
+ INIT_WORK(&s->p[i].tx_work, max310x_tx_proc);
/* Initialize queue for changing LOOPBACK mode */
INIT_WORK(&s->p[i].md_work, max310x_md_proc);
/* Initialize queue for changing RS485 mode */
@@ -1350,8 +1343,6 @@ static int max310x_probe(struct device *dev, struct max310x_devtype *devtype,
}
}
- mutex_destroy(&s->mutex);
-
out_clk:
clk_disable_unprepare(s->clk);
@@ -1372,7 +1363,6 @@ static int max310x_remove(struct device *dev)
s->devtype->power(&s->p[i].port, 0);
}
- mutex_destroy(&s->mutex);
clk_disable_unprepare(s->clk);
return 0;
--
2.21.0
^ permalink raw reply related
* [PATCH 0/7] tty: max310x: Simplify the code and fix a few bugs
From: Serge Semin @ 2019-05-14 10:14 UTC (permalink / raw)
To: Greg Kroah-Hartman, Jiri Slaby; +Cc: Serge Semin, linux-serial, linux-kernel
I started using this driver two years ago in kernek 4.4 and then in kernel
4.9. It didn't go well from the very beginning due to my platform
peculiarities: DW SPI core with hardware CS and relatively slow MIPS-based
SoC. This patchset is intended to fix some of the problems I found out
during the max310x driver utilization with max14830 device.
First of all it was discovered, that workqueue API isn't optimally used.
Work context isn't re-entrant by design, so the mutex used to guard the
TX-method is redundant. schedule_work() method is also created in a way
the work item is scheduled only if it isn't pending. Patch 1 concerns all
these fixes. Seeing the similar container_of(uart_port) is used three
times in the driver, the patch 2 introduces a macro to_max310x_port() to
get a pointer to corresponding struct max310x_one. This is the code
simplification and is going to be used in the following patches.
It was found out, that batch read and write methods used buffers allocated
on the kernel stack. Since they might be utilized by SPI controllers for
DMA it might be unsafe on some platforms. Patch 3 provides a dedicated
kmalloced buffers for this.
The baud-rate calculator function didn't work correct for all the possible
baud-rates requested within a pre-defined input reference frequency.
Instead an algo fully compliant with datasheet divisor formulae is
implemented in patch 4.
Patches 5 and 6 are created to fix some rs485 issues. Particularly the
rs485 mode is configured on the port startup if it's enabled. And seeing
the mode2 register provides a way to enable/disable the echo-suppression
in RS485 mode, it is used to implement the SER_RS485_RX_DURING_TX flag
support.
Finally it was discovered that in case if inbound hardware FIFO
experienced overflow, a lot of '\0' characters inserted into the
flip-buffer as a character of the RX-FIFO overrun. It isn't quite correct
since the overflow happened only after the last character had been
received. Patch 7 is dedicated to push only a single RX-FIFO overrun
character in this case.
Signed-off-by: Serge Semin <fancer.lancer@gmail.com>
Serge Semin (7):
tty: max310x: Simplify tx-work item code
tty: max310x: Introduce max310x_one port macro-wrapper
tty: max310x: Don't pass stacked buffers to SPI
tty: max310x: Fix invalid baudrate divisors calculator
tty: max310x: Add rx-during-tx rs485 flag support
tty: max310x: Optionally enable rs485 on startup
tty: max310x: Split uart characters insertion loop
drivers/tty/serial/max310x.c | 157 +++++++++++++++++++++--------------
1 file changed, 95 insertions(+), 62 deletions(-)
--
2.21.0
^ permalink raw reply
* Re: [PATCH] serial: 8250: Add support for using platform_device resources
From: Andy Shevchenko @ 2019-05-14 9:37 UTC (permalink / raw)
To: Esben Haabendal
Cc: Andy Shevchenko, Lee Jones, open list:SERIAL DRIVERS,
Enrico Weigelt, Greg Kroah-Hartman, Jiri Slaby, Darwin Dingel,
Jisheng Zhang, Sebastian Andrzej Siewior, He Zhe, Marek Vasut,
Douglas Anderson, Paul Burton, Linux Kernel Mailing List
In-Reply-To: <CAHp75VfrP6SLVzmp6LepN7dU1c7QYxfRDRtj7dCTuWzmYp2tCA@mail.gmail.com>
On Tue, May 14, 2019 at 12:23 PM Andy Shevchenko
<andy.shevchenko@gmail.com> wrote:
> On Tue, May 14, 2019 at 10:24 AM Esben Haabendal <esben@haabendal.dk> wrote:
> > Please take a look at https://lkml.org/lkml/2019/4/9/576
> > ("[PATCH v2 2/4] mfd: ioc3: Add driver for SGI IOC3 chip")
>
> Thank you for this link.
> Now, look at this comment:
>
> + /*
> + * Map all IOC3 registers. These are shared between subdevices
> + * so the main IOC3 module manages them.
> + */
>
> Is it your case? Can we see the code?
They do not request resources by the way.
You may do the same, I told you this several times.
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH] serial: 8250: Add support for using platform_device resources
From: Andy Shevchenko @ 2019-05-14 9:23 UTC (permalink / raw)
To: Esben Haabendal
Cc: Andy Shevchenko, Lee Jones, open list:SERIAL DRIVERS,
Enrico Weigelt, Greg Kroah-Hartman, Jiri Slaby, Darwin Dingel,
Jisheng Zhang, Sebastian Andrzej Siewior, He Zhe, Marek Vasut,
Douglas Anderson, Paul Burton, Linux Kernel Mailing List
In-Reply-To: <87k1etmrfk.fsf@haabendal.dk>
On Tue, May 14, 2019 at 10:24 AM Esben Haabendal <esben@haabendal.dk> wrote:
> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> > On Tue, May 07, 2019 at 02:22:18PM +0200, Esben Haabendal wrote:
> We are on repeat here. I don't agree with you here. I have a simple
> generic 8250 (16550A) compatible device, and cannot use it in a mfd
> driver using the standard mfd-core framework.
> The lacking of support for platform_get_resource() in the generic
> serial8250 driver is not a feature. It should be supported, just as it
> is in several of the specialized 8250 drivers.
We are going circles here.
What exactly prevents you to use it? Presence of request_mem_region()?
> It would still mean that I would have revert to not using convenient and
> otherwise fully appropriate API calls like pci_request_regions() and
> mfd_add_devices().
Yes, here is the issue. 8250 requires the parent not to *request*
resources. Because child handles IO access itself.
> The mfd driver in question is for a PCI device. Not being able to
> request the PCI regions seems silly.
Nope. Otherwise, the parent which *doesn't handle* IO on behalf of
child should not request its resources.
> Not being able to register all child devices with the call introduced
> for that sole purpose also seems silly.
> Please take a look at https://lkml.org/lkml/2019/4/9/576
> ("[PATCH v2 2/4] mfd: ioc3: Add driver for SGI IOC3 chip")
Thank you for this link.
Now, look at this comment:
+ /*
+ * Map all IOC3 registers. These are shared between subdevices
+ * so the main IOC3 module manages them.
+ */
Is it your case? Can we see the code?
--
With Best Regards,
Andy Shevchenko
^ permalink raw reply
* Re: [PATCH v2] serial: sh-sci: disable DMA for uart_console
From: Geert Uytterhoeven @ 2019-05-14 8:28 UTC (permalink / raw)
To: George G. Davis
Cc: Eugeniu Rosca, Simon Horman, Wolfram Sang, Greg Kroah-Hartman,
Jiri Slaby, open list:SERIAL DRIVERS, open list, Chris Brandt,
Ulrich Hecht, Andy Lowe, Linux-Renesas,
OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS, Magnus Damm,
Rob Herring, Mark Rutland, stable
In-Reply-To: <1557762446-23811-1-git-send-email-george_davis@mentor.com>
Hi George,
On Mon, May 13, 2019 at 5:48 PM George G. Davis <george_davis@mentor.com> wrote:
> As noted in commit 84b40e3b57ee ("serial: 8250: omap: Disable DMA for
> console UART"), UART console lines use low-level PIO only access functions
> which will conflict with use of the line when DMA is enabled, e.g. when
> the console line is also used for systemd messages. So disable DMA
> support for UART console lines.
>
> Fixes: https://patchwork.kernel.org/patch/10929511/
I don't think this is an appropriate reference, as it points to a patch that
was never applied.
As the problem has basically existed forever, IMHO no Fixes tag
is needed.
> Reported-by: Michael Rodin <mrodin@de.adit-jv.com>
> Tested-by: Eugeniu Rosca <erosca@de.adit-jv.com>
> Reviewed-by: Simon Horman <horms+renesas@verge.net.au>
> Reviewed-by: Wolfram Sang <wsa+renesas@sang-engineering.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: George G. Davis <george_davis@mentor.com>
> ---
> v2: Clarify comment regarding DMA support on kernel console,
> add {Tested,Reviewed}-by:, and Cc: linux-stable lines.
Thanks for the update!
Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be>
Gr{oetje,eeting}s,
Geert
--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
^ permalink raw reply
* Re: [PATCH 2/2] serial: 8250: Add support for 8250/16550 as MFD function
From: Esben Haabendal @ 2019-05-14 8:00 UTC (permalink / raw)
To: Lee Jones
Cc: linux-serial, Greg Kroah-Hartman, Jiri Slaby, Nishanth Menon,
Vignesh R, Tony Lindgren, Lokesh Vutla, Florian Fainelli,
linux-kernel
In-Reply-To: <20190507133844.GA6194@dell>
Lee Jones <lee.jones@linaro.org> writes:
> On Tue, 07 May 2019, Esben Haabendal wrote:
>
>> Lee Jones <lee.jones@linaro.org> writes:
>>
>> > On Fri, 26 Apr 2019, Esben Haabendal wrote:
>> >
>> >> The serial8250-mfd driver is for adding 8250/16550 UART ports as functions
>> >> to an MFD driver.
>> >>
>> >> When calling mfd_add_device(), platform_data should be a pointer to a
>> >> struct plat_serial8250_port, with proper settings like .flags, .type,
>> >> .iotype, .regshift and .uartclk. Memory (or ioport) and IRQ should be
>> >> passed as cell resources.
>> >
>> > What? No, please!
>> >
>> > If you *must* create a whole driver just to be able to use
>> > platform_*() helpers (which I don't think you should), then please
>> > call it something else. This doesn't have anything to do with MFD.
>>
>> True.
>>
>> I really don't think it is a good idea to create a whole driver just to
>> be able to use platform_get_*() helpers. And if I am forced to do this,
>> because I am unable to convince Andy to improve the standard serial8250
>> driver to support that, it should be called MFD. The driver would be
>
> I assume you mean "shouldn't"?
Of-course.
>> generally usable for all usecases where platform_get_*() works.
>>
>> I don't have any idea what to call such a driver. It really would just
>> be a fork of the current serial8250 driver, just allowing use of
>> platform_get_*(), supporting exactly the same hardware.
>>
>> I am still hoping that we can find a way to improve serial8250 to be
>> usable in these cases.
>
> Me too.
Unfortunately, I don't seem to be able to convince Andy to accept
something like that.
I might have to do this out-of-tree :(
/Esben
^ permalink raw reply
* Re: [RFC v2 08/11] arm64: dts: sdm845: Add ufs opps and power-domains
From: Ulf Hansson @ 2019-05-14 7:53 UTC (permalink / raw)
To: Rajendra Nayak
Cc: Linux Kernel Mailing List, linux-arm-msm, Linux PM, linux-serial,
linux-spi, dri-devel, linux-scsi, Stephen Boyd, Viresh Kumar,
Doug Anderson, Rafael J. Wysocki
In-Reply-To: <20190320094918.20234-9-rnayak@codeaurora.org>
On Wed, 20 Mar 2019 at 10:50, Rajendra Nayak <rnayak@codeaurora.org> wrote:
>
> Add the additional power domain and the OPP table for ufs on sdm845
> so the driver can set the appropriate performance state of the
> power domain while setting the clock rate.
>
> Signed-off-by: Rajendra Nayak <rnayak@codeaurora.org>
> ---
> arch/arm64/boot/dts/qcom/sdm845.dtsi | 20 +++++++++++++++++++-
> 1 file changed, 19 insertions(+), 1 deletion(-)
>
> diff --git a/arch/arm64/boot/dts/qcom/sdm845.dtsi b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> index 027ffe6e93e8..a3af4a1757b4 100644
> --- a/arch/arm64/boot/dts/qcom/sdm845.dtsi
> +++ b/arch/arm64/boot/dts/qcom/sdm845.dtsi
> @@ -1140,6 +1140,21 @@
> };
> };
>
> + ufs_opp_table: ufs-opp-table {
> + compatible = "operating-points-v2";
> +
> + opp-50000000 {
> + opp-hz = /bits/ 64 <50000000>;
> + required-opps = <&rpmhpd_opp_min_svs>;
> + };
> +
> + opp-200000000 {
> + opp-hz = /bits/ 64 <200000000>;
> + required-opps = <&rpmhpd_opp_nom>;
> +
> + };
> + };
> +
> ufs_mem_hc: ufshc@1d84000 {
> compatible = "qcom,sdm845-ufshc", "qcom,ufshc",
> "jedec,ufs-2.0";
> @@ -1148,7 +1163,7 @@
> phys = <&ufs_mem_phy_lanes>;
> phy-names = "ufsphy";
> lanes-per-direction = <2>;
> - power-domains = <&gcc UFS_PHY_GDSC>;
> + power-domains = <&gcc UFS_PHY_GDSC>, <&rpmhpd SDM845_CX>;
You probably want to use "power-domain-names" as well.
[...]
Kind regards
Uffe
^ permalink raw reply
* Re: [PATCH] serial: 8250: Add support for using platform_device resources
From: Esben Haabendal @ 2019-05-14 7:37 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Lee Jones, linux-serial, Enrico Weigelt, Greg Kroah-Hartman,
Jiri Slaby, Darwin Dingel, Jisheng Zhang,
Sebastian Andrzej Siewior, He Zhe, Marek Vasut, Douglas Anderson,
Paul Burton, linux-kernel, Thomas Bogendoerfer
In-Reply-To: <20190507150847.GW9224@smile.fi.intel.com>
Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> On Tue, May 07, 2019 at 02:22:18PM +0200, Esben Haabendal wrote:
>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>> > On Tue, May 07, 2019 at 01:35:58PM +0200, Esben Haabendal wrote:
>> >> Lee Jones <lee.jones@linaro.org> writes:
>> >> > On Thu, 02 May 2019, Esben Haabendal wrote:
>> >> >
>> >> >> Could you help clarify whether or not this patch is trying to do
>> >> >> something odd/wrong?
>> >> >>
>> >> >> I might be misunderstanding Andy (probably is), but the discussion
>> >> >> revolves around the changes I propose where I change the serial8250
>> >> >> driver to use platform_get_resource() in favour of
>> >> >> request_mem_region()/release_mem_region().
>> >> >
>> >> > Since 'serial8250' is registered as a platform device, I don't see any
>> >> > reason why it shouldn't have the capability to obtain its memory
>> >> > regions from the platform_get_*() helpers.
>> >>
>> >> Good to hear. That is exactly what I am trying do with this patch.
>> >>
>> >> @Andy: If you still don't like my approach, could you please advice an
>> >> acceptable method for improving the serial8250 driver to allow the use
>> >> of platform_get_*() helpers?
>> >
>> > I still don't get why you need this.
>>
>> Because platform_get_resource() is a generally available and useful
>> helper function for working with platform_device resources, that the
>> current standard serial8250 driver does not support.
>>
>> I am uncertain if I still haven't convinced you that current serial8250
>> driver does not work with platform_get_resource(), or if you believe
>> that it really should not support it.
>
> I believe there is no need to do this support.
>
> Most of the platform code that uses it is quite legacy, and all under arch/
> ideally should be converted to use Device Tree.
Please take a look at https://lkml.org/lkml/2019/4/9/576
("[PATCH v2 2/4] mfd: ioc3: Add driver for SGI IOC3 chip")
This is basically what I am trying to do. I am just so unfortunate that
the serial devices I have are completely generic, so it does not make
sense for me to create a specialized 8250 driver.
Look at how the serial8250_ioc3_driver uses platform_get_resource() to
get the register memory, and how that works together with
mfd_add_devices() in the mfd driver. Nice and elegant. Standard
recommended approach for an mfd driver.
/Esben
^ permalink raw reply
* Re: [PATCH] tty: serial_core: Fix the incorrect configuration of baud rate and data length at the console serial port resume
From: Johan Hovold @ 2019-05-14 7:34 UTC (permalink / raw)
To: Lanqing Liu, Rob Herring, Greg Kroah-Hartman
Cc: baolin.wang, baolin.wang, jslaby, lanqing.liu, linux-serial,
linux-kernel, chunyan.zhang, orson.zhai, zhang.lyra
In-Reply-To: <a8bd7d19de25b799098659334de7e19670a806fc.1557379676.git.liuhhome@gmail.com>
On Thu, May 09, 2019 at 01:42:39PM +0800, Lanqing Liu wrote:
> When userspace opens a serial port for console, uart_port_startup()
> is called. This function assigns the uport->cons->cflag value to
> TTY->termios.c_cflag, then it is cleared to 0. When the user space
> closes this serial port, the TTY structure will be released, and at
> this time uport->cons->cflag has also been cleared.
>
> On the Spreadtrum platform, in some special scenarios, like charging mode,
> userspace needs to close the console, which means the uport->cons->cflag
> has also been cleared. But printing logs is still needed in the kernel. So
> when system enters suspend and resume, the console needs to be configure
> the baud rate and data length of the serial port according to its own cflag
> when resuming the console port. At this time, the cflag is 0, which will
> cause serial port to produce configuration errors that do not meet user
> expectations.
This is actually yet another regression due to 761ed4a94582 ("tty:
serial_core: convert uart_close to use tty_port_close") which
incidentally removed the call to uart_shutdown() where the cflag was
being saved precisely to avoid the problem you're describing:
ae84db9661ca ("serial: core: Preserve termios c_cflag for console resume")
Judging from a quick look it seems the xmit buf, which is released in
that function may now be leaking too.
> To fix this, assigning the TTY->termios.c_cflag value to uport->cons->cflag
> before the userspace closes this console serial port. It will ensure that
> the correct cflag value can be gotten when the console serial port was
> resumed.
Not sure this is the right fix, but I don't have time to look at this
right now.
Johan
^ permalink raw reply
* Re: [PATCH] serial: 8250: Add support for using platform_device resources
From: Esben Haabendal @ 2019-05-14 7:22 UTC (permalink / raw)
To: Andy Shevchenko
Cc: Lee Jones, linux-serial, Enrico Weigelt, Greg Kroah-Hartman,
Jiri Slaby, Darwin Dingel, Jisheng Zhang,
Sebastian Andrzej Siewior, He Zhe, Marek Vasut, Douglas Anderson,
Paul Burton, linux-kernel
In-Reply-To: <20190507150847.GW9224@smile.fi.intel.com>
Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
> On Tue, May 07, 2019 at 02:22:18PM +0200, Esben Haabendal wrote:
>> Andy Shevchenko <andriy.shevchenko@linux.intel.com> writes:
>> > On Tue, May 07, 2019 at 01:35:58PM +0200, Esben Haabendal wrote:
>> >> Lee Jones <lee.jones@linaro.org> writes:
>> >> > On Thu, 02 May 2019, Esben Haabendal wrote:
>> >> >
>> >> >> Could you help clarify whether or not this patch is trying to do
>> >> >> something odd/wrong?
>> >> >>
>> >> >> I might be misunderstanding Andy (probably is), but the discussion
>> >> >> revolves around the changes I propose where I change the serial8250
>> >> >> driver to use platform_get_resource() in favour of
>> >> >> request_mem_region()/release_mem_region().
>> >> >
>> >> > Since 'serial8250' is registered as a platform device, I don't see any
>> >> > reason why it shouldn't have the capability to obtain its memory
>> >> > regions from the platform_get_*() helpers.
>> >>
>> >> Good to hear. That is exactly what I am trying do with this patch.
>> >>
>> >> @Andy: If you still don't like my approach, could you please advice an
>> >> acceptable method for improving the serial8250 driver to allow the use
>> >> of platform_get_*() helpers?
>> >
>> > I still don't get why you need this.
>>
>> Because platform_get_resource() is a generally available and useful
>> helper function for working with platform_device resources, that the
>> current standard serial8250 driver does not support.
>>
>> I am uncertain if I still haven't convinced you that current serial8250
>> driver does not work with platform_get_resource(), or if you believe
>> that it really should not support it.
>
> I believe there is no need to do this support.
>
> Most of the platform code that uses it is quite legacy,
So all code that use/support platform_get_resource() is legacy code?
commit 7945f929f1a77a1c8887a97ca07f87626858ff42
Author: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Date: Wed Feb 20 11:12:39 2019 +0000
drivers: provide devm_platform_ioremap_resource()
There are currently 1200+ instances of using platform_get_resource()
and devm_ioremap_resource() together in the kernel tree.
This patch wraps these two calls in a single helper. Thanks to that
we don't have to declare a local variable for struct resource * and can
omit the redundant argument for resource type. We also have one
function call less.
Signed-off-by: Bartosz Golaszewski <bgolaszewski@baylibre.com>
Acked-by: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reviewed-by: Andy Shevchenko <andriy.shevchenko@linux.intel.com>
Signed-off-by: Linus Walleij <linus.walleij@linaro.org>
It does not looks quite dead to me.
> and all under arch/
> ideally should be converted to use Device Tree.
When do you expect arch/x86 to be converted to device tree?
>> > If it's MFD, you may use "serial8250" with a given platform data like
>> > dozens of current users do.
>>
>> There is only one in-tree mfd driver using "serial8250", the sm501.c
>> driver. And that driver predates the mfd framework (mfd-core.c) by a
>> year, and does not use any of the mfd-core functionality.
>
> So, does it have an issue?
I don't have hardware so I can test it, but I assume that it is
working.
It is ignoring framework code (mfd-core), that is implemented to await
re-inventing the wheel for each and every mfd driver. If that is an
issue, then yes, sm501.c does have an issue and could be improved/fixed.
>> I want to use the mfd-core provided handling of resource splitting,
>> because it makes it easier to handle splitting of a single memory
>> resource as defined by a PCI BAR in this case. And the other drivers I
>> need to use all support/use platform_get_resource(), so it would even
>> have an impact on the integration of that if I cannot use mfd resource
>> splitting with serial8250.
>
> I tired to repeat, that is OKAY! You *may* split and supply resources to the
> drivers, nothing prevents you to do that with current code base.
>
> Do you see any problem with that? What is that problem?
>
> If you would like utilize serial8250, just provide a platform data for
> it.
I fear we are coming to an end here.
I don't seem to be able to break through to you, to get you to
understand the issue here.
I want to write a simple and elegant mfd driver, using mfd-core
framework (the mfd_add_devices() function call to be specific). I don't
want to reimplement similar functionality in the mfd driver.
The other drivers I need all work fine with this, but serial8250 does
not.
As I understand Lee Jones, he seem to agree with me, so could you
please, please consider that I might not be totally on crack, and might
actually have brough forward a valid proposition.
>> > Another approach is to use 8250 library, thus, creating a specific
>> > glue driver (like all 8250_* do).
>>
>> As mentioned, I think this is a bad approach, and I would prefer to
>> improve the "serial8250" driver instead. But if you insist, what should
>> I call such a driver? It needs a platform_driver name, for use when
>> matching with platform_device devices. And it would support exactly the
>> same hardware as the current "serial8250" driver.
>
> If you need some specifics, you create a driver with whatever name
> suits the IP in question. Nevertheless, if it's simple generic 8250, nothing
> needs to be added, except platform data, see above.
We are on repeat here. I don't agree with you here. I have a simple
generic 8250 (16550A) compatible device, and cannot use it in a mfd
driver using the standard mfd-core framework.
The lacking of support for platform_get_resource() in the generic
serial8250 driver is not a feature. It should be supported, just as it
is in several of the specialized 8250 drivers.
>> > Yes, I understand that 8250 driver is full of quirks and not modern
>> > approaches
>> > to do one or another thing. Unfortunately it's not too easy to fix it
>> > without
>> > uglifying code and doing some kind of ping-pong thru the conversion. I don't
>> > think it worth to do it in the current state of affairs. Though, cleaning up
>> > the core part from the quirks and custom pieces would make this task
>> > achievable.
>>
>> I think it should be possible and worthwhile to improve serial8250
>> driver with support for using platform_device resources
>> (platform_get_resource() helper).
>
> I simple can't understand why it's needed. What problem would it solve which
> can't be solved with existing code base?
On repeat again. I have explained it way to many times, so I guess I
must assume by now that you do not think that being able to use
serial8250 together with mfd-core is something that should be solved.
>> If we could stop discussing if it is a proper thing to do, we could try
>> to find a good way to do it instead.
>
>> > Btw, what exact IP of UART do you have implemented there?
>>
>> It is an XPS 16550 UART (v3.00a).
>> https://www.xilinx.com/support/documentation/ip_documentation/xps_uart16550.pdf
>
> So, briefly looking at it I didn't find any deviations from a standard 16550a.
Exactly. I am pretty sure I have said this more than once in this
thread. This IP is perfectly standard. It would be completely wrong to
write a specialized 8250 driver for this.
But if I fail to find a way to get something merged which allows using
the generic serial8250 driver with platform_get_resource(), I guess I
need to handle this out-of-tree. And anybody else needing/wanting to do
the same would have to do that as well.
> Also there are two drivers mentioned Xilinx, though I'm pretty sure it's not
> your case.
>
> Since you have more than one of them, it's even smaller to use current
> infrastructure to enumerate them using only one serial8250 description.
> See plenty examples in the Linux kernel, such as 8250_exar_st16c554.c.
> That is what you may just modify for your needs and put inside your MFD.
It would still mean that I would have revert to not using convenient and
otherwise fully appropriate API calls like pci_request_regions() and
mfd_add_devices().
The mfd driver in question is for a PCI device. Not being able to
request the PCI regions seems silly.
Not being able to register all child devices with the call introduced
for that sole purpose also seems silly.
/Esben
^ permalink raw reply
* Re: [PATCH] tty: serial: uartlite: avoid null pointer dereference during rmmod
From: Johan Hovold @ 2019-05-14 7:08 UTC (permalink / raw)
To: Kefeng Wang
Cc: linux-kernel, linux-serial, Peter Korsgaard, Shubhrajyoti Datta,
Greg Kroah-Hartman, Hulk Robot
In-Reply-To: <20190514033219.169947-1-wangkefeng.wang@huawei.com>
On Tue, May 14, 2019 at 11:32:19AM +0800, Kefeng Wang wrote:
> After commit 415b43bdb008 "tty: serial: uartlite: Move uart register to
> probe", calling uart_unregister_driver unconditionally will trigger a
> null pointer dereference due to ulite_uart_driver may not registed.
>
> CPU: 1 PID: 3755 Comm: syz-executor.0 Not tainted 5.1.0+ #28
> Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
> Call Trace:
> __dump_stack lib/dump_stack.c:77 [inline]
> dump_stack+0xa9/0x10e lib/dump_stack.c:113
> __kasan_report+0x171/0x18d mm/kasan/report.c:321
> kasan_report+0xe/0x20 mm/kasan/common.c:614
> tty_unregister_driver+0x19/0x100 drivers/tty/tty_io.c:3383
> uart_unregister_driver+0x30/0xc0 drivers/tty/serial/serial_core.c:2579
> __do_sys_delete_module kernel/module.c:1027 [inline]
> __se_sys_delete_module kernel/module.c:970 [inline]
> __x64_sys_delete_module+0x244/0x330 kernel/module.c:970
> do_syscall_64+0x72/0x2a0 arch/x86/entry/common.c:298
> entry_SYSCALL_64_after_hwframe+0x49/0xbe
>
> Call uart_unregister_driver only if ulite_uart_driver.state not null to
> fix it.
>
> Cc: Peter Korsgaard <jacmet@sunsite.dk>
> Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
> Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
> Reported-by: Hulk Robot <hulkci@huawei.com>
> Fixes: 415b43bdb008 ("tty: serial: uartlite: Move uart register to probe")
> Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
> ---
> drivers/tty/serial/uartlite.c | 3 ++-
> 1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
> index b8b912b5a8b9..06e79c11141d 100644
> --- a/drivers/tty/serial/uartlite.c
> +++ b/drivers/tty/serial/uartlite.c
> @@ -897,7 +897,8 @@ static int __init ulite_init(void)
> static void __exit ulite_exit(void)
> {
> platform_driver_unregister(&ulite_platform_driver);
> - uart_unregister_driver(&ulite_uart_driver);
> + if (ulite_uart_driver.state)
> + uart_unregister_driver(&ulite_uart_driver);
> }
>
> module_init(ulite_init);
This looks like you're just papering over the real issue, which is the
crazy idea of ultimately registering one driver per port:
https://lkml.kernel.org/r/1539685088-13465-1-git-send-email-shubhrajyoti.datta@gmail.com
It appears only the preparatory patches from that series were applied,
and I think whoever is responsible should consider reverting those
instead.
If the statically allocated port state is that big of any issue, you
need to make serial core support dynamic allocation.
Johan
^ permalink raw reply
* [PATCH] tty: serial: uartlite: avoid null pointer dereference during rmmod
From: Kefeng Wang @ 2019-05-14 3:32 UTC (permalink / raw)
To: linux-kernel, linux-serial
Cc: Kefeng Wang, Peter Korsgaard, Shubhrajyoti Datta,
Greg Kroah-Hartman, Hulk Robot
After commit 415b43bdb008 "tty: serial: uartlite: Move uart register to
probe", calling uart_unregister_driver unconditionally will trigger a
null pointer dereference due to ulite_uart_driver may not registed.
CPU: 1 PID: 3755 Comm: syz-executor.0 Not tainted 5.1.0+ #28
Hardware name: QEMU Standard PC (i440FX + PIIX, 1996), BIOS 1.10.2-1ubuntu1 04/01/2014
Call Trace:
__dump_stack lib/dump_stack.c:77 [inline]
dump_stack+0xa9/0x10e lib/dump_stack.c:113
__kasan_report+0x171/0x18d mm/kasan/report.c:321
kasan_report+0xe/0x20 mm/kasan/common.c:614
tty_unregister_driver+0x19/0x100 drivers/tty/tty_io.c:3383
uart_unregister_driver+0x30/0xc0 drivers/tty/serial/serial_core.c:2579
__do_sys_delete_module kernel/module.c:1027 [inline]
__se_sys_delete_module kernel/module.c:970 [inline]
__x64_sys_delete_module+0x244/0x330 kernel/module.c:970
do_syscall_64+0x72/0x2a0 arch/x86/entry/common.c:298
entry_SYSCALL_64_after_hwframe+0x49/0xbe
Call uart_unregister_driver only if ulite_uart_driver.state not null to
fix it.
Cc: Peter Korsgaard <jacmet@sunsite.dk>
Cc: Shubhrajyoti Datta <shubhrajyoti.datta@xilinx.com>
Cc: Greg Kroah-Hartman <gregkh@linuxfoundation.org>
Reported-by: Hulk Robot <hulkci@huawei.com>
Fixes: 415b43bdb008 ("tty: serial: uartlite: Move uart register to probe")
Signed-off-by: Kefeng Wang <wangkefeng.wang@huawei.com>
---
drivers/tty/serial/uartlite.c | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
index b8b912b5a8b9..06e79c11141d 100644
--- a/drivers/tty/serial/uartlite.c
+++ b/drivers/tty/serial/uartlite.c
@@ -897,7 +897,8 @@ static int __init ulite_init(void)
static void __exit ulite_exit(void)
{
platform_driver_unregister(&ulite_platform_driver);
- uart_unregister_driver(&ulite_uart_driver);
+ if (ulite_uart_driver.state)
+ uart_unregister_driver(&ulite_uart_driver);
}
module_init(ulite_init);
--
2.20.1
^ permalink raw reply related
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