Re: [PATCH] serial: 8250: add option to disable registration of legacy ISA ports
On 2021-01-31 14:18, Måns Rullgård wrote: What userspace visable change will be caused by this? There won't be /dev/ttyS devices for ports that don't exist. Oh yes, please! That would mean I can create ttyS2 and upward while keeping ttyPS0 and ttyPS1 (which invaded the serial alias namespace) at the lower numbers. Will ports get renumbered? Not if they had predictable numbers to begin with. It is hard to make predictable numbers with the backward operating serial aliases in the device tree. It makes relations in the wrong direction. If a system has two ttyPS uarts (xilinx_uartps) and needs them at ttyPS0 and ttyPS1 (or at least <=ttyPS9, due to another bug in start_getty) and 10 ttyS (8250) you can configure the kernel for 10 8250 devices and give 8 of them the predictable ttyS2 to ttyS9. The last two will get the remaining ttyS0 and ttyS1. You cannot assign them their number, because the serial0 and serial1 alias are required for the ttyPS0 and ttyPS1. However with this option it would be possible to configure for 12 8250 devices and not use nor see ttyS0 and ttyS1. The best option would of course be to not even instantiate kernel drivers for non-existing devices. Maarten
Re: [PATCH v2 0/7] tty: add flag to suppress ready signalling on open
On 2020-12-10 19:59, Mychaela Falconia wrote: Maarten Brock wrote: I agree. And an application not configuring the required handshakes, but still relying on them is an equal bug. This comment can be interpreted in at least two different ways. Are you referring to: 1) Mainstream existing applications that expect DTR and/or RTS to be asserted on open without doing any explicit TIOCMBIS, or 2) My fc-host-tools programs (fc-loadtool, fc-xram, rvinterf etc) that operate on the second UART of my DUART28C adapter, expect to NOT have auto-assertion of DTR/RTS on open, but rely on my custom USB ID and the ftdi_sio driver patch that goes with it to suppress this auto- assertion, without doing any explicit TIOCMBIC. If you are referring to 1, it is difficult to fault the authors of those applications, as they had every right to depend on the behaviour that had been codified in numerous official standards like POSIX. Or if you are referring to 2, what other choice do I have? With existing unpatched Linux kernels of every currently existing version, it is *impossible* to prevent DTR & RTS auto-assertion immediately on open of a tty device, thus applying a patch to the kernel (or at least to the ftdi_sio driver in my case) is the *only* way to make my hardware work with Linux. Doing a TIOCMBIC after open won't help, as it will be too late if the kernel already asserted DTR & RTS and thus caused an unwanted deep reset. I was referring to 1) And I still think that an application that *relies* on handshakes working should configure the handshakes, even if POSIX promises they should be set up. Any device with a classic old-fashioned RS-232 has probably already solved this in another way or is accepted as not working on Linux. If someone built such a device for their own personal enjoyment rather than for commercial sale, and needed to get it working with Linux, I suspect that person most likely applied a local patch to the kernel on their own system, likely implementing something similar to what is being discussed in this thread. A person might have done that, a company will probably just not support Linux or do a redesign with a different solution. Personally, I would prefer the VID:PID to enforce the quirk and an O_DIRECT (or other) flag used on open() as general backup plan. To me a sysfs solution seems illogical. A sysfs solution could work as a sort of poor man's substitute for a VID:PID-driven quirk: instead of a driver quirk in the kernel, there is a udev rule that detects a particular USB-serial device (perhaps based on textual manuf/product strings as opposed to VID:PID) and sets the needed sysfs flag. But then if we are talking about a special USB device as opposed to generic serial as in RS-232 etc, then I argue for a driver quirk: if the device has a custom VID:PID, a patch to the driver is needed in any case just to recognize that custom ID, so why not support the custom hw device properly by setting the quirk bit right there and then? Seen in this light, the sysfs approach indeed makes little sense. OTOH, if we are talking about RS-232 or similarly interfaced devices which the user plugs into any random serial port (PC native, or a random off-the-shelf USB-serial cable), then there is really nothing that a udev rule can key onto, it is just a user plugging in some serial device and then running custom userspace apps on it. In this case asking the user to 'echo' something from the shell into /sys/blah prior to running her userspace app seems illogical indeed, and asking userspace app programmers to implement an equivalent sysfs write internally is equally awkward. For this non-custom-USB-ID scenario I thus agree that the O_DIRECT approach would be better - in this case the userspace app programmer simply needs to add this one flag to their open call, a trivial one line change. Or use your option 3) mentioned below: open with O_DIRECT, use ioctl to set the sticky flag and close before starting the application. O_DIRECT is an interesting hack, has anyone seen if it violates the posix rules for us to use it on a character device like this? According to open(2) Linux man page, O_DIRECT does not come from POSIX at all, instead it is specific to Linux, FreeBSD and SGI IRIX. Thus it seems like there aren't any POSIX rules to be violated here. If we go with O_DIRECT, what semantics are we going to implement? There are 3 possibilities that come to mind most readily: 1) O_DIRECT applies only to the open call in which this flag is set, and suppresses DTR/RTS assertion on that open. If someone needs to do multiple opens with DTR/RTS suppression being required every time, then they need to include O_DIRECT every time. 2) O_DIRECT applies not only immediately, but also sets a latched flag whereby all subsequent opens continue to suppress auto-assertion without requiring O_DIRECT every time. This approach by itself runs counter to the generic Uni
Re: [PATCH v2 0/7] tty: add flag to suppress ready signalling on open
On 2020-12-10 11:50, Greg Kroah-Hartman wrote: On Thu, Dec 10, 2020 at 11:41:24AM +0100, Maarten Brock wrote: Hello Mychaela, On 2020-12-09 23:49, Mychaela Falconia wrote: > Greg K-H wrote: > > > I think we need more review for the rest of the series. This does > > change the way serial ports work in a non-traditional way (i.e. using > > sysfs instead of terminal settings). > > But the problem is that the current status quo is fundamentally broken > for those hardware devices in which DTR and/or RTS have been repurposed > for something other than modem and flow control. Right now whenever a > "cold" (never previously opened) serial port is opened for the first > time, that open action immediately and unstoppably asserts both DTR > and RTS hardware outputs, without giving userspace any opportunity to > say "no, please don't do it". Yes, this behaviour is codified in a > bunch of standards that ultimately trace back to 1970s Original UNIX, > but just because it is a standard does not make it right - this > Unix/POSIX/Linux "standard" serial port behaviour is a bug, not a > feature. I agree. And an application not configuring the required handshakes, but still relying on them is an equal bug. > But if there exist some custom hw devices out there that are in the > same predicament as my DUART28 adapter, but are different in that they > are classic old-fashioned RS-232 rather than integrated USB-serial, > with no place to assign a custom USB ID, *then* we need a non-USB-ID- > dependent solution such as Johan's sysfs attribute or O_DIRECT. Any device with a classic old-fashioned RS-232 has probably already solved this in another way or is accepted as not working on Linux. And then there is also the device tree (overlay?) through which a quirk like this can be communicated to the kernel driver. Not sure if this could help for a plug-and-play device like on USB. > > So I want to get a bunch of people > > to agree that this is ok to do things this way now before taking this > > new user-visible api. Personally, I would prefer the VID:PID to enforce the quirk and an O_DIRECT (or other) flag used on open() as general backup plan. To me a sysfs solution seems illogical. The "problem" of a vid:pid is that for usb-serial devices, that only describes the device that does the conversion itself, NOT the serial device the converter is plugged into that cares about these types of line-wiggling. Just like you would not want to classify all devices that met the PCI serial class signature for this type of thing either, there is nothing special about USB here other than it happens to be a common transport for these signals these days. thanks, greg k-h This is true for a generic USB-UART board or cable, but not for a dedicated PCB where both the USB-UART chip and the special connection are implemented and which has a dedicated VID:PID different from any generic one. In this case the VID:PID describes the whole board. If the line-wiggling requirement is created behind some sort of connector (real RS-232 DB9/DB25 or CMOS pin header or whatever) then the problem is the same as for an 8250 on any other bus. For this situation I would prefer the O_DIRECT flag on open(). Maarten
Re: [PATCH v2 0/7] tty: add flag to suppress ready signalling on open
Hello Mychaela, On 2020-12-09 23:49, Mychaela Falconia wrote: Greg K-H wrote: I think we need more review for the rest of the series. This does change the way serial ports work in a non-traditional way (i.e. using sysfs instead of terminal settings). But the problem is that the current status quo is fundamentally broken for those hardware devices in which DTR and/or RTS have been repurposed for something other than modem and flow control. Right now whenever a "cold" (never previously opened) serial port is opened for the first time, that open action immediately and unstoppably asserts both DTR and RTS hardware outputs, without giving userspace any opportunity to say "no, please don't do it". Yes, this behaviour is codified in a bunch of standards that ultimately trace back to 1970s Original UNIX, but just because it is a standard does not make it right - this Unix/POSIX/Linux "standard" serial port behaviour is a bug, not a feature. I agree. And an application not configuring the required handshakes, but still relying on them is an equal bug. But if there exist some custom hw devices out there that are in the same predicament as my DUART28 adapter, but are different in that they are classic old-fashioned RS-232 rather than integrated USB-serial, with no place to assign a custom USB ID, *then* we need a non-USB-ID- dependent solution such as Johan's sysfs attribute or O_DIRECT. Any device with a classic old-fashioned RS-232 has probably already solved this in another way or is accepted as not working on Linux. And then there is also the device tree (overlay?) through which a quirk like this can be communicated to the kernel driver. Not sure if this could help for a plug-and-play device like on USB. So I want to get a bunch of people to agree that this is ok to do things this way now before taking this new user-visible api. Personally, I would prefer the VID:PID to enforce the quirk and an O_DIRECT (or other) flag used on open() as general backup plan. To me a sysfs solution seems illogical. If the concern is with the new sysfs interface or the proposed O_DIRECT alternative, how about deferring those while allowing specific USB ID support to go in first? Right now there already exists at least one piece of hardware actively supported by its manufacturer (my gadget) that has a custom USB ID and requires the quirk - what is wrong with adding support for this existing specific hw? How about merging Johan's patch that defines the NORDY flag in tty_port, merging the ftdi_sio driver patch setting this flag for my custom USB ID, allowing other hardware engineers in the same boat to submit similar quirk patches for their affected custom hw with custom USB IDs, while deferring the sysfs patches until there is a more pressing need for quirky devices that have no custom USB IDs? Sincerely, Mychaela Again, I agree. Maarten
Re: [PATCH 22/36] tty: serial: xilinx_uartps: Supply description for missing member 'cts_override'
On 2020-11-04 20:35, Lee Jones wrote:
Fixes the following W=1 kernel build warning(s):
drivers/tty/serial/xilinx_uartps.c:205: warning: Function parameter
or member 'cts_override' not described in 'cdns_uart'
Cc: Greg Kroah-Hartman
Cc: Jiri Slaby
Cc: Michal Simek
Cc: linux-ser...@vger.kernel.org
Signed-off-by: Lee Jones
---
drivers/tty/serial/xilinx_uartps.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/tty/serial/xilinx_uartps.c
b/drivers/tty/serial/xilinx_uartps.c
index a9b1ee27183a7..a14c5d9964739 100644
--- a/drivers/tty/serial/xilinx_uartps.c
+++ b/drivers/tty/serial/xilinx_uartps.c
@@ -192,6 +192,7 @@ MODULE_PARM_DESC(rx_timeout, "Rx timeout, 1-255");
* @baud: Current baud rate
* @clk_rate_change_nb:Notifier block for clock changes
* @quirks:Flags for RXBS support.
+ * @cts_override: Modem control state override
*/
struct cdns_uart {
struct uart_port*port;
While you are at it, would you consider to also fix the indentation of
the
cts_override flag at line 208?
Maarten
Re: [PATCH v3 3/5] serial: 8250: Support separate rs485 rx-enable GPIO
On 2020-05-18 18:35, Andy Shevchenko wrote: On Mon, May 18, 2020 at 06:13:16PM +0200, Maarten Brock wrote: On 2020-05-18 17:22, Lukas Wunner wrote: > On Mon, May 18, 2020 at 06:12:41PM +0300, Andy Shevchenko wrote: > > On Sun, May 17, 2020 at 11:56:08PM +0200, Heiko Stuebner wrote: > > > From: Heiko Stuebner > > > > > > The RE signal is used to control the duplex mode of transmissions, > > > aka receiving data while sending in full duplex mode, while stopping > > > receiving data in half-duplex mode. > > > > > > On a number of boards the !RE signal is tied to ground so reception > > > is always enabled except if the UART allows disabling the receiver. > > > This can be taken advantage of to implement half-duplex mode - like > > > done on 8250_bcm2835aux. > > > > > > Another solution is to tie !RE to RTS always forcing half-duplex mode. > > > > > > And finally there is the option to control the RE signal separately, > > > like done here by introducing a new rs485-specific gpio that can be > > > set depending on the RX_DURING_TX setting in the common em485 callbacks. > > > > ... > > > > > +port->rs485_re_gpio = devm_gpiod_get_optional(dev, "rs485-rx-enable", > > > + GPIOD_OUT_HIGH); > > > > While reviewing some other patch I realized that people are missing > > the > > point of these GPIO flags when pin is declared to be output. > > > > HIGH here means "asserted" (consider active-high vs. active-low in > > general). Is that the intention here? > > > > Lukas, same question to your patch. > > Yes. "High", i.e. asserted, means "termination enabled" in the case of > my patch and "receiver enabled" in the case of Heiko's patch. But "High" on a gpio would disable the receiver when connected to !RE. No, that's exactly the point of the terminology (asserted means active whatever polarity it is). You need to define active-low in GPIO description. Is there anything wrong with defining GPIOD_OUT_ACTIVE or GPIOD_OUT_ASSERTED for this very purpose? May I suggest to deprecate GPIOD_OUT_HIGH and replace it? Maarten
Re: [PATCH v3 3/5] serial: 8250: Support separate rs485 rx-enable GPIO
On 2020-05-18 17:22, Lukas Wunner wrote: On Mon, May 18, 2020 at 06:12:41PM +0300, Andy Shevchenko wrote: On Sun, May 17, 2020 at 11:56:08PM +0200, Heiko Stuebner wrote: > From: Heiko Stuebner > > The RE signal is used to control the duplex mode of transmissions, > aka receiving data while sending in full duplex mode, while stopping > receiving data in half-duplex mode. > > On a number of boards the !RE signal is tied to ground so reception > is always enabled except if the UART allows disabling the receiver. > This can be taken advantage of to implement half-duplex mode - like > done on 8250_bcm2835aux. > > Another solution is to tie !RE to RTS always forcing half-duplex mode. > > And finally there is the option to control the RE signal separately, > like done here by introducing a new rs485-specific gpio that can be > set depending on the RX_DURING_TX setting in the common em485 callbacks. ... > + port->rs485_re_gpio = devm_gpiod_get_optional(dev, "rs485-rx-enable", > +GPIOD_OUT_HIGH); While reviewing some other patch I realized that people are missing the point of these GPIO flags when pin is declared to be output. HIGH here means "asserted" (consider active-high vs. active-low in general). Is that the intention here? Lukas, same question to your patch. Yes. "High", i.e. asserted, means "termination enabled" in the case of my patch and "receiver enabled" in the case of Heiko's patch. But "High" on a gpio would disable the receiver when connected to !RE. Maarten
Re: [PATCH v2 4/7] serial: 8250: Handle implementations not having TEMT interrupt using em485
On 2020-05-02 15:49, Lukas Wunner wrote: On Thu, Mar 26, 2020 at 12:14:19AM +0100, Heiko Stuebner wrote: Some 8250 ports have a TEMT interrupt but it's not a part of the 8250 standard, instead only available on some implementations. The current em485 implementation does not work on ports without it. The only chance to make it work is to loop-read on LSR register. So add UART_CAP_TEMT to mark 8250 uarts having this interrupt, update all current em485 users with that capability and make the stop_tx function loop-read on uarts not having it. Just to get a better understanding: According to the Dw_apb_uart_db.pdf databook I've found, the UART does have a "THR empty" interrupt. So you get an interrupt once the Transmit Holding Register (and by consequence the FIFO) has been drained. Then what do you need a TEMT interrupt for? Why is the THR interrupt not sufficient? When the Transmit Holding Register is empty, the Transmitter can still be transmitting. And the Driver Enable must be held active during transmission. I would even say it needs to held active during transmission of the stop bit, but I don't believe there is any uart that has an interrupt flag for that. And since the default state for RS485 is '1' anyway it's not that bad. Maarten
Re: [PATCH v2 0/4] serial: uartps: Driver updates
On 2018-12-20 07:52, Michal Simek wrote: Hi, On 19. 12. 18 19:40, Maarten Brock wrote: Hello Michal, On 2018-12-18 13:18, Michal Simek wrote: Hi, I am sending 4 patches in series to fix some issues we found. Patches were sent separately but I have been asked to send them in serial that's why I am also adding cover letter to explain this v2 version. Thanks, Michal I'm wondering why, when reading the linux-serial mailing list, I can only see this cover letter. Are you perhaps using a different To/Cc for the actual patches? And if so, is that on purpose? I have checked linux-serial mailing list and I see all of them there. https://www.spinics.net/lists/linux-serial/ Individual here. https://www.spinics.net/lists/linux-serial/msg32919.html https://www.spinics.net/lists/linux-serial/msg32916.html https://www.spinics.net/lists/linux-serial/msg32917.html https://www.spinics.net/lists/linux-serial/msg32918.html Also I see linux-serial@ in CC in all emails I have sent. Thanks, Michal Thanks, I see them there too. But the odd thing is that I did not receive them in my mailbox. And I had noticed this before that I was missing your posts following a cover letter. I normally do receive other peoples patches after a cover letter. Maarten
Re: [PATCH v2 0/4] serial: uartps: Driver updates
Hello Michal, On 2018-12-18 13:18, Michal Simek wrote: Hi, I am sending 4 patches in series to fix some issues we found. Patches were sent separately but I have been asked to send them in serial that's why I am also adding cover letter to explain this v2 version. Thanks, Michal I'm wondering why, when reading the linux-serial mailing list, I can only see this cover letter. Are you perhaps using a different To/Cc for the actual patches? And if so, is that on purpose? Kind regards, Maarten
Re: [RFC PATCH 0/3] serial: uartps: Add run time support for more IPs than hardcoded 2
On 2018-04-26 16:08, Michal Simek wrote: Hi, this series is trying to address discussion I had with Alan in past https://patchwork.kernel.org/patch/9738445/. It is moving uart_register_driver() to probe function like it is done in pl011 driver. And also introducing new function for alias compatibility checking to resolve cases where some IPs have alias and some of them not. This case is detected in pl011_probe_dt_alias() but not properly solved. Also keep status of free ids/minor numbers in bitmap to know what's the next unallocated number. The same solution can be used in pl011, uart16550 and uartlite to really get unified kernel. Tested on these use cases: Notes: ff00 is the first PS uart listed in dtsi ff01 is the second PS uart listed in dtsi Standard zcu102 setting serial0 = &uart0; serial1 = &uart1; [0.196628] ff00.serial: ttyPS0 at MMIO 0xff00 (irq = 33, base_baud = 625) is a xuartps [0.642542] ff01.serial: ttyPS1 at MMIO 0xff01 (irq = 34, base_baud = 625) is a xuartps Swapped zcu102 setting serial0 = &uart1; serial1 = &uart0; [0.196472] ff00.serial: ttyPS1 at MMIO 0xff00 (irq = 33, base_baud = 625) is a xuartps [0.196824] ff01.serial: ttyPS0 at MMIO 0xff01 (irq = 34, base_baud = 625) is a xuartps uart0 on alias higher then MAX uart ports serial0 = &uart1; serial200 = &uart0; [0.176793] ff00.serial: ttyPS200 at MMIO 0xff00 (irq = 33, base_baud = 625) is a xuartps [0.177288] ff01.serial: ttyPS0 at MMIO 0xff01 (irq = 34, base_baud = 625) is a xuartps Both uarts on higher aliases serial1 = &uart1; serial2 = &uart0; [0.196470] ff00.serial: ttyPS2 at MMIO 0xff00 (irq = 33, base_baud = 625) is a xuartps [0.196823] ff01.serial: ttyPS1 at MMIO 0xff01 (irq = 34, base_baud = 625) is a xuartps uart0 not listed but it is probed first that's why should be ttyPS0 but there is uart1 via alias serial0 = &uart1; [0.176656] xuartps ff00.serial: No serial alias passed. Using the first free id [0.176671] xuartps ff00.serial: Validate id 0 [0.176680] xuartps ff00.serial: The empty id is 0 [0.176692] xuartps ff00.serial: ID 0 already taken - skipped [0.176701] xuartps ff00.serial: Validate id 1 [0.176710] xuartps ff00.serial: The empty id is 1 [0.176719] xuartps ff00.serial: Selected ID 1 allocation passed [0.176760] ff00.serial: ttyPS1 at MMIO 0xff00 (irq = 33, base_baud = 625) is a xuartps [0.177104] ff01.serial: ttyPS0 at MMIO 0xff01 (irq = 34, base_baud = 625) is a xuartps uart0 not listed but it is probed first that's why should be ttyPS0 serial1 = &uart1; [0.176661] xuartps ff00.serial: No serial alias passed. Using the first free id [0.176676] xuartps ff00.serial: Validate id 0 [0.176686] xuartps ff00.serial: The empty id is 0 [0.176696] xuartps ff00.serial: Selected ID 0 allocation passed [0.176737] ff00.serial: ttyPS0 at MMIO 0xff00 (irq = 33, base_baud = 625) is a xuartps [0.177069] ff01.serial: ttyPS1 at MMIO 0xff01 (irq = 34, base_baud = 625) is a xuartps uarts not listed in aliases list [0.176673] xuartps ff00.serial: No serial alias passed. Using the first free id [0.176687] xuartps ff00.serial: Validate id 0 [0.176697] xuartps ff00.serial: The empty id is 0 [0.176707] xuartps ff00.serial: Selected ID 0 allocation passed [0.176746] ff00.serial: ttyPS0 at MMIO 0xff00 (irq = 33, base_baud = 625) is a xuartps [0.177057] xuartps ff01.serial: No serial alias passed. Using the first free id [0.177070] xuartps ff01.serial: Validate id 0 [0.177079] xuartps ff01.serial: The empty id is 0 [0.177089] xuartps ff01.serial: Selected ID 0 allocation failed [0.177098] xuartps ff01.serial: Validate id 1 [0.177107] xuartps ff01.serial: The empty id is 1 [0.177116] xuartps ff01.serial: Selected ID 1 allocation passed [0.177149] ff01.serial: ttyPS1 at MMIO 0xff01 (irq = 34, base_baud = 625) is a xuartps Thanks, Michal Hello Michal, How will this interact with ns16550 based UARTs? Can we have both /dev/ttyS0 and /dev/ttyPS0? Currently we can't unless we create *no* serialN alias for the ns16550. And there is no other means to lock the /dev/ttySx name to a device either. Or will the xuartps driver eventually use /dev/ttySx as well? Maarten
Re: [PATCH 1/1] xilinx ps uart: Adding a kernel parameter for the number of xilinx ps uarts
On 2017-05-24 18:09, Michal Simek wrote: On 24.5.2017 15:31, Alan Cox wrote: I am not saying that config option is perfect solution. It is at least aligned with 5 others serial drivers in the tree. And the fact people keep doing hacks jutifies continuing to make a mess. Especialy as in this case it's entirely theoretical. Nobody has produced actual hardware that hits the limit. Nobody has filed a bug, nobody is impacted. This is the reason why we are talking about it how to do it right. With this ps uart it is not that easy because this is cadence RTL which is not in public IP database but the same think is with uartlite. Limit there is 16. If you really want that I can create that HW design which will require more than 16 uartlites in one design. Creating extra CONFIG_ entries for junk like this is ridiculous, most of the others at least have the excuse of being old code. No doubt about it. I am just trying to find out what's the way you are suggesting. A patch was already recently sent to this mailing list to add a CONFIG_ entry for the maximum number of uartlite devices. It is, as you say, quite easy to create a device with more than 16 uartlite devices. (It is probably harder to create one with many Cadence RTL PS uarts since that would require a license.) IMHO it is quite normal for anyone using the uartlite to build his/her own kernel. and thus make config settings. But it is cumbersome to have to modify the kernel sources. If the number of uartlites could be retrieved from the device tree that would probably be even better if the price in complexity and code size is reasonable. Maarten
Re: [PATCH] Tty: serial - Fix possible NULL derefrence.
Hello Greg,
Does that also mean that this isn't possible in the sc16is7xx.c driver
in
sc16is7xx_spi_probe() line 1358 and sc16is7xx_i2c_probe() line 1419 ?
If so, should these checks be removed?
Kind regards,
Maarten
On 2017-01-30 20:37, Greg Kroah-Hartman wrote:
On Fri, Jan 27, 2017 at 04:46:13PM +0530, Shailendra Verma wrote:
of_match_device could return NULL, and so can cause a NULL
pointer dereference later.
Signed-off-by: Shailendra Verma
---
drivers/tty/serial/max310x.c |4
1 file changed, 4 insertions(+)
diff --git a/drivers/tty/serial/max310x.c
b/drivers/tty/serial/max310x.c
index 8a3e926..a94d147 100644
--- a/drivers/tty/serial/max310x.c
+++ b/drivers/tty/serial/max310x.c
@@ -1323,6 +1323,10 @@ static int max310x_spi_probe(struct spi_device
*spi)
if (spi->dev.of_node) {
const struct of_device_id *of_id =
of_match_device(max310x_dt_ids, &spi->dev);
+ if (!of_id) {
+ dev_err(&spi->dev, "Error: No device match found\n");
+ return -ENODEV;
+ }
Patch now dropped as this isn't possible.
thanks,
greg k-h
--
To unsubscribe from this list: send the line "unsubscribe linux-serial"
in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] sc16is7xx: add reading CTS
Hello Pascal,
If you look at the history of this driver, you'll see that something similar was
present in the past, but was removed because it introduced problems. The problem
is that you can't just call sc16is7xx_port_read() unsynchronized to the worker
thread. And this is related to the device using indirect access through an I2C
or SPI driver with separate interrupts from the UART. There is also more info on
the linux-serial mailing list.
Kind regards,
Maarten Brock
- Original Message -
From: Pascal JEAN [mailto:epsilo...@gmail.com]
To: Greg KH [mailto:gre...@linuxfoundation.org]
Cc: jsl...@suse.com, linux-ser...@vger.kernel.org, linux-kernel@vger.kernel.org
Sent: Thu, 28 Jul 2016 09:07:36 +0200
Subject: [PATCH 2/2] sc16is7xx: add reading CTS
> Hi Greg,
>
> Here are changelog for this patch.
> Best Regards
>
> Changelog:
>
> sc16is7xx: add reading CTS
>
> This patch adds the possibility to read the actual
> status of the CTS input when RTS/CTS handshaking is
> not activated (for general purposes).
>
>
> Signed-off-by: Pascal JEAN
> ---
> drivers/tty/serial/sc16is7xx.c | 17 +
> 1 file changed, 13 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/tty/serial/sc16is7xx.c b/drivers/tty/serial/sc16is7xx.c
> index 3e65079..8833a18 100644
> --- a/drivers/tty/serial/sc16is7xx.c
> +++ b/drivers/tty/serial/sc16is7xx.c
> @@ -818,10 +818,19 @@ static unsigned int sc16is7xx_tx_empty(struct
> uart_port *port)
>
> static unsigned int sc16is7xx_get_mctrl(struct uart_port *port)
> {
> - /* DCD and DSR are not wired and CTS/RTS is handled automatically
> - * so just indicate all inputs asserted
> - */
> - return TIOCM_DSR | TIOCM_CAR | TIOCM_RI | TIOCM_CTS;
> + /* DCD, DSR and RI are not wired so just indicate asserted */
> + unsigned int mctrl = TIOCM_CAR | TIOCM_DSR | TIOCM_RI;
> +
> + if (port->status & UPSTAT_CTS_ENABLE)
> + /* CTS handled automatically, indicates that it is always
> + * asserted, this is required for proper management of
> + * the upper layer
> + */
> + mctrl |= TIOCM_CTS;
> + else {
> + /* CTS is not managed automatically, returns its actual state
> + * the upper layer
> + */
> + u8 msr = sc16is7xx_port_read(port, SC16IS7XX_MSR_REG);
> +
> + mctrl |= (msr & SC16IS7XX_MSR_CTS_BIT) ? TIOCM_CTS : 0;
> + }
> + return mctrl;
> }
>
> static void sc16is7xx_set_mctrl(struct uart_port *port, unsigned int mctrl)
> --
> 2.7.4
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-serial" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
Re: [PATCH v3 03/11] dmaengine: dw: set polarity of handshake interface
- Original Message - From: Andy Shevchenko [mailto:andy.shevche...@gmail.com] To: Maarten Brock [mailto:m.br...@vanmierlo.com] Cc: Andy Shevchenko [mailto:andriy.shevche...@linux.intel.com], Bryan O'Donoghue [mailto:pure.lo...@nexus-software.ie], Peter Hurley [mailto:pe...@hurleysoftware.com], linux-ser...@vger.kernel.org [mailto:linux-ser...@vger.kernel.org], Vinod Koul [mailto:vinod.k...@intel.com], linux-kernel@vger.kernel.org [mailto:linux-kernel@vger.kernel.org], dmaengine [mailto:dmaeng...@vger.kernel.org], Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org], Puustinen, Ismo [mailto:ismo.puusti...@intel.com], Heikki Krogerus [mailto:heikki.kroge...@linux.intel.com] Sent: Fri, 06 May 2016 13:38:44 +0200 Subject: Re: [PATCH v3 03/11] dmaengine: dw: set polarity of handshake interface > On Fri, May 6, 2016 at 2:20 PM, Maarten Brock wrote: > >> > > > >> > > + boolpolarity; > >> > So this variable is not very intuitively named. > >> > >> There is a help above. This is a property of the Synopsys DesignWare DMA > >> engine. Anyone familiar with datasheet easily understands this. > >> > >> > > >> > You end up setting somepointer->polarity = true; in a later patch. > >> > > >> > Since you're respining a V4 I'd suggest a name that describes a little > >> > bit better than polarity. Setting polarity = true is a little bit > >> > liked being asked "you you like ice-cream or apple pie" and then > >> > saying "yes please". > >> > >> It's about handshake interface polarity, so, what about hs_polarity? > > > > So it means: handshake has polarity (true) or handshake has no polarity > > (omnidirectional?) (false), right? > > It means that handshake polarity _signal_ is inverted (true) or > default (false) in terms of hardware gates. Would it not be better then to name it handshake_inverted ? That is something you can ask and answer with true or false. Maarten > -- > With Best Regards, > Andy Shevchenko >
Re: [PATCH v3 03/11] dmaengine: dw: set polarity of handshake interface
- Original Message - From: Andy Shevchenko [mailto:andriy.shevche...@linux.intel.com] To: Bryan O'Donoghue [mailto:pure.lo...@nexus-software.ie], Peter Hurley [mailto:pe...@hurleysoftware.com], linux-ser...@vger.kernel.org, Vinod Koul [mailto:vinod.k...@intel.com], linux-kernel@vger.kernel.org, dmaeng...@vger.kernel.org, Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org], ismo.puusti...@intel.com, Heikki Krogerus [mailto:heikki.kroge...@linux.intel.com] Sent: Fri, 06 May 2016 12:42:00 +0200 Subject: Re: [PATCH v3 03/11] dmaengine: dw: set polarity of handshake interface > On Thu, 2016-05-05 at 18:54 +0100, Bryan O'Donoghue wrote: > > On Wed, 2016-04-27 at 16:48 +0300, Andy Shevchenko wrote: > > > > > > + boolpolarity; > > So this variable is not very intuitively named. > > There is a help above. This is a property of the Synopsys DesignWare DMA > engine. Anyone familiar with datasheet easily understands this. > > > > > You end up setting somepointer->polarity = true; in a later patch. > > > > Since you're respining a V4 I'd suggest a name that describes a little > > bit better than polarity. Setting polarity = true is a little bit > > liked being asked "you you like ice-cream or apple pie" and then > > saying "yes please". > > It's about handshake interface polarity, so, what about hs_polarity? So it means: handshake has polarity (true) or handshake has no polarity (omnidirectional?) (false), right? Maarten
Re: [PATCH] serial-uartlite: un-constify uartlite_be/uartlite_le
- Original Message - From: Greg Kroah-Hartman [mailto:gre...@linuxfoundation.org] To: Maarten Brock [mailto:m.br...@vanmierlo.com] Cc: Arnd Bergmann [mailto:a...@arndb.de], Peter Korsgaard [mailto:jac...@sunsite.dk], Jiri Slaby [mailto:jsl...@suse.com], Rich Felker [mailto:dal...@libc.org], linux-ser...@vger.kernel.org, linux-kernel@vger.kernel.org Sent: Tue, 19 Apr 2016 08:12:36 +0200 Subject: Re: [PATCH] serial-uartlite: un-constify uartlite_be/uartlite_le > On Thu, Mar 10, 2016 at 10:08:01AM +0100, Maarten Brock wrote: > > I've created a version 2 of this patch immediately which fixes the > > warning, but somehow this stays ignored. > > > > Please apply my second patch! > > Sorry, it was too late, my fault. > > greg k-h Hello Greg, Now that the original patch is reverted, can you apply my second version of the patch which includes extra casts to suppress the warnings? Maarten
Re: [PATCH] serial-uartlite: fix build warning
- Original Message -
From: Geert Uytterhoeven [mailto:ge...@linux-m68k.org]
To: Sudip Mukherjee [mailto:sudipm.mukher...@gmail.com]
Cc: Peter Korsgaard [mailto:jac...@sunsite.dk], Greg Kroah-Hartman
[mailto:gre...@linuxfoundation.org], Jiri Slaby [mailto:jsl...@suse.com],
linux-kernel@vger.kernel.org [mailto:linux-kernel@vger.kernel.org],
linux-ser...@vger.kernel.org [mailto:linux-ser...@vger.kernel.org]
Sent: Fri, 18 Mar 2016 13:48:06 +0100
Subject: Re: [PATCH] serial-uartlite: fix build warning
> On Thu, Mar 10, 2016 at 2:14 PM, Sudip Mukherjee
> wrote:
> > We were getting build warnings about:
> > drivers/tty/serial/uartlite.c: In function ‘ulite_request_port’:
> > drivers/tty/serial/uartlite.c:348:21: warning: assignment discards
> > 'const' qualifier from pointer target type
> > port->private_data = &uartlite_be;
> > ^
> > drivers/tty/serial/uartlite.c:354:22: warning: assignment discards
> > 'const' qualifier from pointer target type
> > port->private_data = &uartlite_le;
> > ^
> >
> > Fixes: 2905697a82ea ("serial-uartlite: Constify uartlite_be/uartlite_le")
> > Signed-off-by: Sudip Mukherjee
>
> Acked-by: Geert Uytterhoeven
>
> Gr{oetje,eeting}s,
>
> Geert
Reverting is not the same as fixing.
Rant:
It is a stupid warning IMHO, but being a compiler writer myself (SDCC) I
understand how it can arise. If you assign some const pointer to a void
pointer without an explicit cast gcc does not complain about the complete
loss of type, but it does warn about losing constness. In general I'd say:
make up your mind; either warn about both or don't warn about either.
Maarten
Re: [PATCH] serial-uartlite: fix build warning
I've created a version 2 of this patch immediately which fixes the warning,
but somehow this stays ignored.
Please apply my second patch!
Maarten
> We were getting build warnings about:
> drivers/tty/serial/uartlite.c: In function ‘ulite_request_port’:
> drivers/tty/serial/uartlite.c:348:21: warning: assignment discards
> 'const' qualifier from pointer target type
> port->private_data = &uartlite_be;
> ^
> drivers/tty/serial/uartlite.c:354:22: warning: assignment discards
> 'const' qualifier from pointer target type
> port->private_data = &uartlite_le;
> ^
>
> Fixes: 2905697a82ea ("serial-uartlite: Constify uartlite_be/uartlite_le")
> Signed-off-by: Sudip Mukherjee
> ---
>
> next-20160310 build log at:
> https://travis-ci.org/sudipm-mukherjee/parport/jobs/114988022
>
> drivers/tty/serial/uartlite.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
> index c9fdfc8..1474c57 100644
> --- a/drivers/tty/serial/uartlite.c
> +++ b/drivers/tty/serial/uartlite.c
> @@ -72,7 +72,7 @@ static void uartlite_outbe32(u32 val, void __iomem *addr)
> iowrite32be(val, addr);
> }
>
> -static const struct uartlite_reg_ops uartlite_be = {
> +static struct uartlite_reg_ops uartlite_be = {
> .in = uartlite_inbe32,
> .out = uartlite_outbe32,
> };
> @@ -87,7 +87,7 @@ static void uartlite_outle32(u32 val, void __iomem *addr)
> iowrite32(val, addr);
> }
>
> -static const struct uartlite_reg_ops uartlite_le = {
> +static struct uartlite_reg_ops uartlite_le = {
> .in = uartlite_inle32,
> .out = uartlite_outle32,
> };
> --
> 1.9.1
Re: [PATCH] serial-uartlite: un-constify uartlite_be/uartlite_le
I've created a version 2 of this patch immediately which fixes the warning,
but somehow this stays ignored.
Please apply my second patch!
Maarten
> The patch to make uartlite_be/uartlite_le const was well-intended but
> caused a new build warning:
>
> tty/serial/uartlite.c: In function 'ulite_request_port':
> tty/serial/uartlite.c:348:21: error: assignment discards 'const' qualifier
> from pointer target type [-Werror=discarded-qualifiers]
> tty/serial/uartlite.c:354:22: error: assignment discards 'const' qualifier
> from pointer target type [-Werror=discarded-qualifiers]
>
> It would be nice to allow passing const pointers through
> port->private_data, but that would be way more work, so this
> reverts part of the original commit for now.
>
> A possible alternative might be to pass a structure in the private_data
> that contains a const pointer to the operations, which introduces a little
> extra overhead, or we could just add a cast to a non-const pointer, I'll
> leave that to the maintainer.
>
> Signed-off-by: Arnd Bergmann
> Fixes: 2905697a82ea ("serial-uartlite: Constify uartlite_be/uartlite_le")
> ---
> drivers/tty/serial/uartlite.c | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/tty/serial/uartlite.c b/drivers/tty/serial/uartlite.c
> index c9fdfc8bf47f..1474c5755140 100644
> --- a/drivers/tty/serial/uartlite.c
> +++ b/drivers/tty/serial/uartlite.c
> @@ -72,7 +72,7 @@ static void uartlite_outbe32(u32 val, void __iomem *addr)
> iowrite32be(val, addr);
> }
>
> -static const struct uartlite_reg_ops uartlite_be = {
> +static struct uartlite_reg_ops uartlite_be = {
> .in = uartlite_inbe32,
> .out = uartlite_outbe32,
> };
> @@ -87,7 +87,7 @@ static void uartlite_outle32(u32 val, void __iomem *addr)
> iowrite32(val, addr);
> }
>
> -static const struct uartlite_reg_ops uartlite_le = {
> +static struct uartlite_reg_ops uartlite_le = {
> .in = uartlite_inle32,
> .out = uartlite_outle32,
> };
> --
> 2.7.0
>
>
