[PATCH v2 1/6] serial: 8250: Store to lsr_save_flags after lsr read

[PATCH v2 1/6] serial: 8250: Store to lsr_save_flags after lsr read

[Ilpo Järvinen]

Not all LSR register flags are preserved across reads. Therefore, LSR
readers must store the non-preserved bits into lsr_save_flags.

This fix was initially mixed into feature commit f6f586102add ("serial:
8250: Handle UART without interrupt on TEMT using em485"). However,
that feature change had a flaw and it was reverted to make room for
simpler approach providing the same feature. The embedded fix got
reverted with the feature change.

Re-add the lsr_save_flags fix and properly mark it's a fix.

Fixes: e490c9144cfa ("tty: Add software emulated RS485 support for 8250")
Link: https://lore.kernel.org/all/1d6c31d-d194-9e6a-ddf9-5f29af829f3@linux.intel.com/T/#m1737eef986bd20cf19593e344cebd7b0244945fc
Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Acked-by: Uwe Kleine-König <u.kleine-koenig@penugtronix.de>
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250_port.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index 4998799abae2..c5e0f925f4b6 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1511,6 +1511,8 @@ static inline void __stop_tx(struct uart_8250_port *p)
 		unsigned char lsr = serial_in(p, UART_LSR);
 		u64 stop_delay = 0;
 
+		p->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
+
 		if (!(lsr & UART_LSR_THRE))
 			return;
 		/*
-- 
2.30.2

[PATCH v2 2/6] serial: 8250: Create serial_lsr_in()

[Ilpo Järvinen]

LSR register readers need to be careful in order to not lose bits that
are not preserved across reads. Create a helper that takes care of
storing the non-preserved bits into lsr_save_flags.

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250.h      | 20 ++++++++++++++++++++
 drivers/tty/serial/8250/8250_core.c |  3 +--
 drivers/tty/serial/8250/8250_port.c | 15 ++++-----------
 3 files changed, 25 insertions(+), 13 deletions(-)

diff --git a/drivers/tty/serial/8250/8250.h b/drivers/tty/serial/8250/8250.h
index 3d9a7e539dea..b120da57c61f 100644
--- a/drivers/tty/serial/8250/8250.h
+++ b/drivers/tty/serial/8250/8250.h
@@ -123,6 +123,26 @@ static inline void serial_out(struct uart_8250_port *up, int offset, int value)
 	up->port.serial_out(&up->port, offset, value);
 }
 
+/**
+ *	serial_lsr_in - Read LSR register and preserve flags across reads
+ *	@up:	uart 8250 port
+ *
+ *	Read LSR register and handle saving non-preserved flags across reads.
+ *	The flags that are not preserved across reads are stored into
+ *	up->lsr_saved_flags.
+ *
+ *	Returns LSR value or'ed with the preserved flags (if any).
+ */
+static inline unsigned int serial_lsr_in(struct uart_8250_port *up)
+{
+	unsigned int lsr = up->lsr_saved_flags;
+
+	lsr |= serial_in(up, UART_LSR);
+	up->lsr_saved_flags = lsr & LSR_SAVE_FLAGS;
+
+	return lsr;
+}
+
 /*
  * For the 16C950
  */
diff --git a/drivers/tty/serial/8250/8250_core.c b/drivers/tty/serial/8250/8250_core.c
index 12c5e5d0ce6d..90ddc8924811 100644
--- a/drivers/tty/serial/8250/8250_core.c
+++ b/drivers/tty/serial/8250/8250_core.c
@@ -276,8 +276,7 @@ static void serial8250_backup_timeout(struct timer_list *t)
 	 * the "Diva" UART used on the management processor on many HP
 	 * ia64 and parisc boxes.
 	 */
-	lsr = serial_in(up, UART_LSR);
-	up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
+	lsr = serial_lsr_in(up);
 	if ((iir & UART_IIR_NO_INT) && (up->ier & UART_IER_THRI) &&
 	    (!uart_circ_empty(&up->port.state->xmit) || up->port.x_char) &&
 	    (lsr & UART_LSR_THRE)) {
diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index c5e0f925f4b6..ec5abeb638eb 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1508,11 +1508,9 @@ static inline void __stop_tx(struct uart_8250_port *p)
 	struct uart_8250_em485 *em485 = p->em485;
 
 	if (em485) {
-		unsigned char lsr = serial_in(p, UART_LSR);
+		unsigned char lsr = serial_lsr_in(p);
 		u64 stop_delay = 0;
 
-		p->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
-
 		if (!(lsr & UART_LSR_THRE))
 			return;
 		/*
@@ -1567,10 +1565,8 @@ static inline void __start_tx(struct uart_port *port)
 
 	if (serial8250_set_THRI(up)) {
 		if (up->bugs & UART_BUG_TXEN) {
-			unsigned char lsr;
+			unsigned char lsr = serial_lsr_in(up);
 
-			lsr = serial_in(up, UART_LSR);
-			up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
 			if (lsr & UART_LSR_THRE)
 				serial8250_tx_chars(up);
 		}
@@ -2001,8 +1997,7 @@ static unsigned int serial8250_tx_empty(struct uart_port *port)
 	serial8250_rpm_get(up);
 
 	spin_lock_irqsave(&port->lock, flags);
-	lsr = serial_port_in(port, UART_LSR);
-	up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
+	lsr = serial_lsr_in(up);
 	spin_unlock_irqrestore(&port->lock, flags);
 
 	serial8250_rpm_put(up);
@@ -2078,9 +2073,7 @@ static void wait_for_lsr(struct uart_8250_port *up, int bits)
 
 	/* Wait up to 10ms for the character(s) to be sent. */
 	for (;;) {
-		status = serial_in(up, UART_LSR);
-
-		up->lsr_saved_flags |= status & LSR_SAVE_FLAGS;
+		status = serial_lsr_in(up);
 
 		if ((status & bits) == bits)
 			break;
-- 
2.30.2

[PATCH v2 3/6] serial: 8250: Get preserved flags using serial_lsr_in()

[Ilpo Järvinen]

serial8250_handle_irq() assumes it's the first to read LSR register.
However, there are 8250 drivers which perform LSR read in their own irq
handler prior to calling serial8250_handle_irq(). As not all flags are
preserved across LSR reads, use serial_lsr_in() helper to get all the
preserved flags.

This commit might fix other commits too besides the ones for DW UART
mentioned below. It's just not clear to me which of the other devices
clear some of the LSR flags on read. AFAIK, nobody has complained about
this problem (either against DW or other devices) so it might not have
that bad impact in the end.

Fixes: 424d79183af0 ("serial: 8250_dw: Avoid "too much work" from bogus rx timeout interrupt")
Fixes: aa63d786cea2 ("serial: 8250: dw: Add support for DMA flow controlling devices")
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250_port.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index ec5abeb638eb..a0ea048eb2ad 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1916,7 +1916,7 @@ int serial8250_handle_irq(struct uart_port *port, unsigned int iir)
 
 	spin_lock_irqsave(&port->lock, flags);
 
-	status = serial_port_in(port, UART_LSR);
+	status = serial_lsr_in(up);
 
 	/*
 	 * If port is stopped and there are no error conditions in the
-- 
2.30.2

[PATCH v2 4/6] serial: 8250: Adjust misleading LSR related comment

[Ilpo Järvinen]

serial8250_rx_chars() has max_count based character limit. If it
triggers, the function returns old LSR value (and it has never returned
only flags which were not handled). Adjust the comment to match
behavior and warn about which flags can be depended on.

While I'd have moved LSR read before LSR read and used serial_lsr_in()
also here but I came across this old discussion about the topic:
  https://www.spinics.net/lists/linux-serial/msg20555.html
...so I left it as it is (it works as long as the callers only use
a subset of the LSR flags which holds true today).

Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250_port.c | 9 ++++++---
 1 file changed, 6 insertions(+), 3 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
index a0ea048eb2ad..686891f1b2ca 100644
--- a/drivers/tty/serial/8250/8250_port.c
+++ b/drivers/tty/serial/8250/8250_port.c
@@ -1782,9 +1782,12 @@ void serial8250_read_char(struct uart_8250_port *up, unsigned char lsr)
 EXPORT_SYMBOL_GPL(serial8250_read_char);
 
 /*
- * serial8250_rx_chars: processes according to the passed in LSR
- * value, and returns the remaining LSR bits not handled
- * by this Rx routine.
+ * serial8250_rx_chars: Read characters. The first LSR value must be passed
+ * in.
+ *
+ * Returns LSR bits. The caller should rely only non-rx related LSR bits
+ * (such as THRE) because the LSR value might come from an already consumed
+ * character.
  */
 unsigned char serial8250_rx_chars(struct uart_8250_port *up, unsigned char lsr)
 {
-- 
2.30.2

[PATCH v2 5/6] serial: 8250_dw: Use serial_lsr_in() in dw8250_handle_irq()

[Ilpo Järvinen]

dw8250_handle_irq() reads LSR under a few conditions, convert both to
use serial_lsr_in() in order to preserve LSR flags properly across
reads.

Cc: Douglas Anderson <dianders@chromium.org>
Cc: Phil Edworthy <phil.edworthy@renesas.com>
Cc: Miquel Raynal <miquel.raynal@bootlin.com>
Fixes: 424d79183af0 ("serial: 8250_dw: Avoid "too much work" from bogus rx timeout interrupt")
Fixes: aa63d786cea2 ("serial: 8250: dw: Add support for DMA flow controlling devices")
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250_dw.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index f57bbd32ef11..1fae45991812 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -253,7 +253,7 @@ static int dw8250_handle_irq(struct uart_port *p)
 	 */
 	if (!up->dma && rx_timeout) {
 		spin_lock_irqsave(&p->lock, flags);
-		status = p->serial_in(p, UART_LSR);
+		status = serial_lsr_in(up);
 
 		if (!(status & (UART_LSR_DR | UART_LSR_BI)))
 			(void) p->serial_in(p, UART_RX);
@@ -263,7 +263,7 @@ static int dw8250_handle_irq(struct uart_port *p)
 
 	/* Manually stop the Rx DMA transfer when acting as flow controller */
 	if (quirks & DW_UART_QUIRK_IS_DMA_FC && up->dma && up->dma->rx_running && rx_timeout) {
-		status = p->serial_in(p, UART_LSR);
+		status = serial_lsr_in(up);
 		if (status & (UART_LSR_DR | UART_LSR_BI)) {
 			dw8250_writel_ext(p, RZN1_UART_RDMACR, 0);
 			dw8250_writel_ext(p, DW_UART_DMASA, 1);
-- 
2.30.2

[PATCH v2 6/6] serial: 8250_dw: Store LSR into lsr_saved_flags in dw8250_tx_wait_empty()

[Ilpo Järvinen]

Make sure LSR flags are preserved in dw8250_tx_wait_empty(). This
function is called from a low-level out function and therefore cannot
call serial_lsr_in() as it would lead to infinite recursion.

It is borderline if the flags need to be saved here at all since this
code relates to writing LCR register which usually implies no important
characters should be arriving.

Fixes: 914eaf935ec7 ("serial: 8250_dw: Allow TX FIFO to drain before writing to UART_LCR")
Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
---
 drivers/tty/serial/8250/8250_dw.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/tty/serial/8250/8250_dw.c b/drivers/tty/serial/8250/8250_dw.c
index 1fae45991812..4cc69bb612ab 100644
--- a/drivers/tty/serial/8250/8250_dw.c
+++ b/drivers/tty/serial/8250/8250_dw.c
@@ -122,12 +122,15 @@ static void dw8250_check_lcr(struct uart_port *p, int value)
 /* Returns once the transmitter is empty or we run out of retries */
 static void dw8250_tx_wait_empty(struct uart_port *p)
 {
+	struct uart_8250_port *up = up_to_u8250p(p);
 	unsigned int tries = 20000;
 	unsigned int delay_threshold = tries - 1000;
 	unsigned int lsr;
 
 	while (tries--) {
 		lsr = readb (p->membase + (UART_LSR << p->regshift));
+		up->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
+
 		if (lsr & UART_LSR_TEMT)
 			break;
 
-- 
2.30.2

Re: [PATCH v2 1/6] serial: 8250: Store to lsr_save_flags after lsr read

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1777 bytes --]

On Mon, 6 Jun 2022, Ilpo Järvinen wrote:

> Not all LSR register flags are preserved across reads. Therefore, LSR
> readers must store the non-preserved bits into lsr_save_flags.
> 
> This fix was initially mixed into feature commit f6f586102add ("serial:
> 8250: Handle UART without interrupt on TEMT using em485"). However,
> that feature change had a flaw and it was reverted to make room for
> simpler approach providing the same feature. The embedded fix got
> reverted with the feature change.
> 
> Re-add the lsr_save_flags fix and properly mark it's a fix.
> 
> Fixes: e490c9144cfa ("tty: Add software emulated RS485 support for 8250")
> Link: https://lore.kernel.org/all/1d6c31d-d194-9e6a-ddf9-5f29af829f3@linux.intel.com/T/#m1737eef986bd20cf19593e344cebd7b0244945fc
> Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> Acked-by: Uwe Kleine-König <u.kleine-koenig@penugtronix.de>

It seems that Uwe managed to mistype his email for the A-by which I just 
happily copy-pasted.

Greg, please let me know if you want me to resend the series (or if you 
will just change it on the fly while applying).

-- 
 i.

> Signed-off-by: Ilpo Järvinen <ilpo.jarvinen@linux.intel.com>
> ---
>  drivers/tty/serial/8250/8250_port.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/tty/serial/8250/8250_port.c b/drivers/tty/serial/8250/8250_port.c
> index 4998799abae2..c5e0f925f4b6 100644
> --- a/drivers/tty/serial/8250/8250_port.c
> +++ b/drivers/tty/serial/8250/8250_port.c
> @@ -1511,6 +1511,8 @@ static inline void __stop_tx(struct uart_8250_port *p)
>  		unsigned char lsr = serial_in(p, UART_LSR);
>  		u64 stop_delay = 0;
>  
> +		p->lsr_saved_flags |= lsr & LSR_SAVE_FLAGS;
> +
>  		if (!(lsr & UART_LSR_THRE))
>  			return;
>  		/*
> 

Re: [PATCH v2 1/6] serial: 8250: Store to lsr_save_flags after lsr read

[Andy Shevchenko]

On Mon, Jun 6, 2022 at 3:55 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
> On Mon, 6 Jun 2022, Ilpo Järvinen wrote:

...

> > Fixes: e490c9144cfa ("tty: Add software emulated RS485 support for 8250")
> > Link: https://lore.kernel.org/all/1d6c31d-d194-9e6a-ddf9-5f29af829f3@linux.intel.com/T/#m1737eef986bd20cf19593e344cebd7b0244945fc
> > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > Acked-by: Uwe Kleine-König <u.kleine-koenig@penugtronix.de>
>
> It seems that Uwe managed to mistype his email for the A-by which I just
> happily copy-pasted.
>
> Greg, please let me know if you want me to resend the series (or if you
> will just change it on the fly while applying).

I believe Greg doesn't handle patches on the fly, hence resend.
But more importantly I do not see the reason for the Acked-by tag when
SoB of the same person is present.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/6] serial: 8250: Store to lsr_save_flags after lsr read

[Ilpo Järvinen]

[-- Attachment #1: Type: text/plain, Size: 1473 bytes --]

On Mon, 6 Jun 2022, Andy Shevchenko wrote:

> On Mon, Jun 6, 2022 at 3:55 PM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> > On Mon, 6 Jun 2022, Ilpo Järvinen wrote:
> 
> ...
> 
> > > Fixes: e490c9144cfa ("tty: Add software emulated RS485 support for 8250")
> > > Link: https://lore.kernel.org/all/1d6c31d-d194-9e6a-ddf9-5f29af829f3@linux.intel.com/T/#m1737eef986bd20cf19593e344cebd7b0244945fc
> > > Signed-off-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
> > > Acked-by: Uwe Kleine-König <u.kleine-koenig@penugtronix.de>
> >
> > It seems that Uwe managed to mistype his email for the A-by which I just
> > happily copy-pasted.
> >
> > Greg, please let me know if you want me to resend the series (or if you
> > will just change it on the fly while applying).
> 
> I believe Greg doesn't handle patches on the fly, hence resend.
> But more importantly I do not see the reason for the Acked-by tag when
> SoB of the same person is present.

I just repeated what Uwe gave me. Maybe he didn't notice he was already 
there as SoB.

This situation is anyway a bit more complex than usual. The line I took 
was part of Uwe's much larger patch initially (which was fully reverted) 
so his SoB was carried over to preserve the authorship. As I made a 
non-trivial modification to his original patch by removing almost all of 
it, I added my SoB too. Given this situation, I kind of thought he Acked 
(approved) the post-modification version of it.

-- 
 i.

Re: [PATCH v2 1/6] serial: 8250: Store to lsr_save_flags after lsr read

[Andy Shevchenko]

On Mon, Jun 6, 2022 at 6:54 PM Ilpo Järvinen
<ilpo.jarvinen@linux.intel.com> wrote:
> On Mon, 6 Jun 2022, Andy Shevchenko wrote:
> > On Mon, Jun 6, 2022 at 3:55 PM Ilpo Järvinen
> > <ilpo.jarvinen@linux.intel.com> wrote:

...

> > But more importantly I do not see the reason for the Acked-by tag when
> > SoB of the same person is present.
>
> I just repeated what Uwe gave me. Maybe he didn't notice he was already
> there as SoB.
>
> This situation is anyway a bit more complex than usual. The line I took
> was part of Uwe's much larger patch initially (which was fully reverted)
> so his SoB was carried over to preserve the authorship. As I made a
> non-trivial modification to his original patch by removing almost all of
> it, I added my SoB too. Given this situation, I kind of thought he Acked
> (approved) the post-modification version of it.

I believe you haven't preserved the authorship that way (since From
line is different), but since you have done non-trivial changes and
Uwe is okay with them, the straightforward tag chain would be (with
your authorship implied):
Co-developed-by: Uwe
SoB: Uwe
SoB: yours

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/6] serial: 8250: Store to lsr_save_flags after lsr read

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 1768 bytes --]

On Mon, Jun 06, 2022 at 07:01:15PM +0200, Andy Shevchenko wrote:
> On Mon, Jun 6, 2022 at 6:54 PM Ilpo Järvinen
> <ilpo.jarvinen@linux.intel.com> wrote:
> > On Mon, 6 Jun 2022, Andy Shevchenko wrote:
> > > On Mon, Jun 6, 2022 at 3:55 PM Ilpo Järvinen
> > > <ilpo.jarvinen@linux.intel.com> wrote:
> 
> ...
> 
> > > But more importantly I do not see the reason for the Acked-by tag when
> > > SoB of the same person is present.
> >
> > I just repeated what Uwe gave me. Maybe he didn't notice he was already
> > there as SoB.
> >
> > This situation is anyway a bit more complex than usual. The line I took
> > was part of Uwe's much larger patch initially (which was fully reverted)
> > so his SoB was carried over to preserve the authorship. As I made a
> > non-trivial modification to his original patch by removing almost all of
> > it, I added my SoB too. Given this situation, I kind of thought he Acked
> > (approved) the post-modification version of it.
> 
> I believe you haven't preserved the authorship that way (since From
> line is different), but since you have done non-trivial changes and
> Uwe is okay with them, the straightforward tag chain would be (with
> your authorship implied):
> Co-developed-by: Uwe
> SoB: Uwe
> SoB: yours

I don't care much, but IMHO the initial set of tags made sense to me. It
has my S-o-b because the change is (somewhat) taken from me and it has
my ack because the modification looked good to me.

But indeed, fixing the email address would be appreciated. Do with the
tags whatever you consider sensible.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/6] serial: 8250: Store to lsr_save_flags after lsr read

[Andy Shevchenko]

On Mon, Jun 6, 2022 at 9:40 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
>
> On Mon, Jun 06, 2022 at 07:01:15PM +0200, Andy Shevchenko wrote:
> > On Mon, Jun 6, 2022 at 6:54 PM Ilpo Järvinen
> > <ilpo.jarvinen@linux.intel.com> wrote:
> > > On Mon, 6 Jun 2022, Andy Shevchenko wrote:
> > > > On Mon, Jun 6, 2022 at 3:55 PM Ilpo Järvinen
> > > > <ilpo.jarvinen@linux.intel.com> wrote:
> >
> > ...
> >
> > > > But more importantly I do not see the reason for the Acked-by tag when
> > > > SoB of the same person is present.
> > >
> > > I just repeated what Uwe gave me. Maybe he didn't notice he was already
> > > there as SoB.
> > >
> > > This situation is anyway a bit more complex than usual. The line I took
> > > was part of Uwe's much larger patch initially (which was fully reverted)
> > > so his SoB was carried over to preserve the authorship. As I made a
> > > non-trivial modification to his original patch by removing almost all of
> > > it, I added my SoB too. Given this situation, I kind of thought he Acked
> > > (approved) the post-modification version of it.
> >
> > I believe you haven't preserved the authorship that way (since From
> > line is different), but since you have done non-trivial changes and
> > Uwe is okay with them, the straightforward tag chain would be (with
> > your authorship implied):
> > Co-developed-by: Uwe
> > SoB: Uwe
> > SoB: yours
>
> I don't care much, but IMHO the initial set of tags made sense to me.

> It
> has my S-o-b because the change is (somewhat) taken from me and it has
> my ack because the modification looked good to me.

According to
https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
the SoB already implies that you developed that, but Ack if not. It
also clarifies Co-developed-by for cases like this.


-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/6] serial: 8250: Store to lsr_save_flags after lsr read

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2706 bytes --]

Hello Andy,

On Mon, Jun 06, 2022 at 10:38:37PM +0200, Andy Shevchenko wrote:
> On Mon, Jun 6, 2022 at 9:40 PM Uwe Kleine-König
> <u.kleine-koenig@pengutronix.de> wrote:
> >
> > On Mon, Jun 06, 2022 at 07:01:15PM +0200, Andy Shevchenko wrote:
> > > On Mon, Jun 6, 2022 at 6:54 PM Ilpo Järvinen
> > > <ilpo.jarvinen@linux.intel.com> wrote:
> > > > On Mon, 6 Jun 2022, Andy Shevchenko wrote:
> > > > > On Mon, Jun 6, 2022 at 3:55 PM Ilpo Järvinen
> > > > > <ilpo.jarvinen@linux.intel.com> wrote:
> > >
> > > ...
> > >
> > > > > But more importantly I do not see the reason for the Acked-by tag when
> > > > > SoB of the same person is present.
> > > >
> > > > I just repeated what Uwe gave me. Maybe he didn't notice he was already
> > > > there as SoB.
> > > >
> > > > This situation is anyway a bit more complex than usual. The line I took
> > > > was part of Uwe's much larger patch initially (which was fully reverted)
> > > > so his SoB was carried over to preserve the authorship. As I made a
> > > > non-trivial modification to his original patch by removing almost all of
> > > > it, I added my SoB too. Given this situation, I kind of thought he Acked
> > > > (approved) the post-modification version of it.
> > >
> > > I believe you haven't preserved the authorship that way (since From
> > > line is different), but since you have done non-trivial changes and
> > > Uwe is okay with them, the straightforward tag chain would be (with
> > > your authorship implied):
> > > Co-developed-by: Uwe
> > > SoB: Uwe
> > > SoB: yours
> >
> > I don't care much, but IMHO the initial set of tags made sense to me.
> 
> > It
> > has my S-o-b because the change is (somewhat) taken from me and it has
> > my ack because the modification looked good to me.
> 
> According to
> https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
> the SoB already implies that you developed that, but Ack if not. It
> also clarifies Co-developed-by for cases like this.

That's unintuitive (and wrong) in my opinion. For me, Acked-by is a
confirmation of the respective person, that the patch in question is ok.
If I take a hunk of a random reverted patch and add the S-o-b of the big
patch's author, can I really assume the original author "acks" the
result? I would expect that in most cases they don't. (And if they do,
there is no way to record it, because the usual way of adding an Ack is
blocked as there is already a S-o-b?)

*shrug*
Uwe
 

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/6] serial: 8250: Store to lsr_save_flags after lsr read

[Andy Shevchenko]

+Cc: Jonathan (some documentation clarification might be needed)

On Tue, Jun 7, 2022 at 7:58 AM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Mon, Jun 06, 2022 at 10:38:37PM +0200, Andy Shevchenko wrote:
> > On Mon, Jun 6, 2022 at 9:40 PM Uwe Kleine-König
> > <u.kleine-koenig@pengutronix.de> wrote:
> > >
> > > On Mon, Jun 06, 2022 at 07:01:15PM +0200, Andy Shevchenko wrote:
> > > > On Mon, Jun 6, 2022 at 6:54 PM Ilpo Järvinen
> > > > <ilpo.jarvinen@linux.intel.com> wrote:
> > > > > On Mon, 6 Jun 2022, Andy Shevchenko wrote:
> > > > > > On Mon, Jun 6, 2022 at 3:55 PM Ilpo Järvinen
> > > > > > <ilpo.jarvinen@linux.intel.com> wrote:
> > > >
> > > > ...
> > > >
> > > > > > But more importantly I do not see the reason for the Acked-by tag when
> > > > > > SoB of the same person is present.
> > > > >
> > > > > I just repeated what Uwe gave me. Maybe he didn't notice he was already
> > > > > there as SoB.
> > > > >
> > > > > This situation is anyway a bit more complex than usual. The line I took
> > > > > was part of Uwe's much larger patch initially (which was fully reverted)
> > > > > so his SoB was carried over to preserve the authorship. As I made a
> > > > > non-trivial modification to his original patch by removing almost all of
> > > > > it, I added my SoB too. Given this situation, I kind of thought he Acked
> > > > > (approved) the post-modification version of it.
> > > >
> > > > I believe you haven't preserved the authorship that way (since From
> > > > line is different), but since you have done non-trivial changes and
> > > > Uwe is okay with them, the straightforward tag chain would be (with
> > > > your authorship implied):
> > > > Co-developed-by: Uwe
> > > > SoB: Uwe
> > > > SoB: yours
> > >
> > > I don't care much, but IMHO the initial set of tags made sense to me.
> >
> > > It
> > > has my S-o-b because the change is (somewhat) taken from me and it has
> > > my ack because the modification looked good to me.
> >
> > According to
> > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
> > the SoB already implies that you developed that, but Ack if not. It
> > also clarifies Co-developed-by for cases like this.
>
> That's unintuitive (and wrong) in my opinion.

I have the opposite opinion.

> For me, Acked-by is a
> confirmation of the respective person, that the patch in question is ok.
> If I take a hunk of a random reverted patch and add the S-o-b of the big
> patch's author, can I really assume the original author "acks" the
> result? I would expect that in most cases they don't. (And if they do,
> there is no way to record it, because the usual way of adding an Ack is
> blocked as there is already a S-o-b?)

It's very logical to me. If you allowed (by not NAKing) the other
developer to use your SoB you imply Ack for every change they made.
Otherwise you need explicitly ask for withdrawal of your SoB.

-- 
With Best Regards,
Andy Shevchenko

Re: [PATCH v2 1/6] serial: 8250: Store to lsr_save_flags after lsr read

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2377 bytes --]

Hello,

On Tue, Jun 07, 2022 at 12:09:39PM +0200, Andy Shevchenko wrote:
> > > > > I believe you haven't preserved the authorship that way (since From
> > > > > line is different), but since you have done non-trivial changes and
> > > > > Uwe is okay with them, the straightforward tag chain would be (with
> > > > > your authorship implied):
> > > > > Co-developed-by: Uwe
> > > > > SoB: Uwe
> > > > > SoB: yours
> > > >
> > > > I don't care much, but IMHO the initial set of tags made sense to me.
> > >
> > > > It
> > > > has my S-o-b because the change is (somewhat) taken from me and it has
> > > > my ack because the modification looked good to me.
> > >
> > > According to
> > > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
> > > the SoB already implies that you developed that, but Ack if not. It
> > > also clarifies Co-developed-by for cases like this.

Reading that by the letter, it doesn't say you must not use Ack if there
is a S-o-b.

	If a person was not directly involved in the preparation or
	handling of a patch but wishes to signify and record their
	approval of it then they can ask to have an Acked-by: line added
	to the patch’s changelog.

It's "If" and not "Iff". Not sure if that is intended?!

> > That's unintuitive (and wrong) in my opinion.
> 
> I have the opposite opinion.
> 
> > For me, Acked-by is a
> > confirmation of the respective person, that the patch in question is ok.
> > If I take a hunk of a random reverted patch and add the S-o-b of the big
> > patch's author, can I really assume the original author "acks" the
> > result? I would expect that in most cases they don't. (And if they do,
> > there is no way to record it, because the usual way of adding an Ack is
> > blocked as there is already a S-o-b?)
> 
> It's very logical to me. If you allowed (by not NAKing) the other
> developer to use your SoB you imply Ack for every change they made.

So you assume that you notice each patch with your S-o-b in time to send
a NAK. I don't claim that for me and I would be surprised if a major
part of the kernel contributors did.

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH v2 1/6] serial: 8250: Store to lsr_save_flags after lsr read

[Andy Shevchenko]

On Tue, Jun 7, 2022 at 12:32 PM Uwe Kleine-König
<u.kleine-koenig@pengutronix.de> wrote:
> On Tue, Jun 07, 2022 at 12:09:39PM +0200, Andy Shevchenko wrote:

...

> > > > > > I believe you haven't preserved the authorship that way (since From
> > > > > > line is different), but since you have done non-trivial changes and
> > > > > > Uwe is okay with them, the straightforward tag chain would be (with
> > > > > > your authorship implied):
> > > > > > Co-developed-by: Uwe
> > > > > > SoB: Uwe
> > > > > > SoB: yours
> > > > >
> > > > > I don't care much, but IMHO the initial set of tags made sense to me.
> > > >
> > > > > It
> > > > > has my S-o-b because the change is (somewhat) taken from me and it has
> > > > > my ack because the modification looked good to me.
> > > >
> > > > According to
> > > > https://www.kernel.org/doc/html/latest/process/submitting-patches.html#when-to-use-acked-by-cc-and-co-developed-by
> > > > the SoB already implies that you developed that, but Ack if not. It
> > > > also clarifies Co-developed-by for cases like this.
>
> Reading that by the letter, it doesn't say you must not use Ack if there
> is a S-o-b.
>
>         If a person was not directly involved in the preparation or
>         handling of a patch but wishes to signify and record their
>         approval of it then they can ask to have an Acked-by: line added
>         to the patch’s changelog.
>
> It's "If" and not "Iff". Not sure if that is intended?!

Yes, it's a bit ambiguous, but I use common sense. Acking yourself
code seems awkward to me.

...

> > > That's unintuitive (and wrong) in my opinion.
> >
> > I have the opposite opinion.
> >
> > > For me, Acked-by is a
> > > confirmation of the respective person, that the patch in question is ok.
> > > If I take a hunk of a random reverted patch and add the S-o-b of the big
> > > patch's author, can I really assume the original author "acks" the
> > > result? I would expect that in most cases they don't. (And if they do,
> > > there is no way to record it, because the usual way of adding an Ack is
> > > blocked as there is already a S-o-b?)
> >
> > It's very logical to me. If you allowed (by not NAKing) the other
> > developer to use your SoB you imply Ack for every change they made.
>
> So you assume that you notice each patch with your S-o-b in time to send
> a NAK. I don't claim that for me and I would be surprised if a major
> part of the kernel contributors did.

Yes. That's why `git send-email` is always Cc'ing people whose SoB
tags in the patch.

The flaw that you may notice here is that anybody can potentially add
anybody's SoB to any garbage and send it to a mailing list. It has
non-zero chances to pass review (a person whose SoB has been abused on
a sick leave, for example). But this is more process and QA related.

-- 
With Best Regards,
Andy Shevchenko