linux-kernel - Re: [PATCH RFC 2/2] serial: 8250: Add PRUSS UART driver

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <aB3qWWgWfs6fDTgg@smile.fi.intel.com>
Date: Fri, 9 May 2025 14:43:21 +0300
From: Andy Shevchenko <andriy.shevchenko@...ux.intel.com>
To: Judith Mendez <jm@...com>
Cc: Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Kevin Hilman <khilman@...libre.com>,
	Jiri Slaby <jirislaby@...nel.org>, linux-kernel@...r.kernel.org,
	linux-serial@...r.kernel.org, Hari Nagalla <hnagalla@...com>
Subject: Re: [PATCH RFC 2/2] serial: 8250: Add PRUSS UART driver

On Thu, May 08, 2025 at 05:09:03PM -0500, Judith Mendez wrote:
> On 5/2/25 4:50 AM, Andy Shevchenko wrote:
> > On Wed, Apr 30, 2025 at 07:31:13PM -0500, Judith Mendez wrote:

...

> > The list of the inclusions is semi-random. Please, follow the IWYU principle.

This and other comments left unanswered, why? What does this mean? Usual way is
to remove the context with all what you are agree on and discuss only stuff
that needs more elaboration.

Ah, I see now the P.S., but please also remove the context you agree with next
time.

> > > +#include <linux/clk.h>
> > > +#include <linux/module.h>
> > > +#include <linux/serial_reg.h>
> > > +#include <linux/serial_core.h>
> > 
> > > +#include <linux/of_irq.h>
> > > +#include <linux/of_address.h>
> > > +#include <linux/of_platform.h>
> > 
> > Please, no of*.h in a new code.
> 
> Will only keep linux/of_platform.h for of_device_id struct.

Hmm... Is it really the header where it's defined? (I know the answer,
but want you to perform some research.)

> > > +#include <linux/remoteproc.h>
> > 
> > Keep them ordered as well.
> > 
> > + blank line here.
> > 
> > > +#include "8250.h"

...

> > > +	/* Old custom speed handling */
> > > +	if (baud == 38400 && (port->flags & UPF_SPD_MASK) == UPF_SPD_CUST) {
> > > +		quot = port->custom_divisor & UART_DIV_MAX;
> > > +		if (port->custom_divisor & (1 << 16))
> > > +			*frac = PRUSS_UART_MDR_13X_MODE;
> > > +		else
> > > +			*frac = PRUSS_UART_MDR_16X_MODE;
> > > +
> > > +		return quot;
> > > +	}
> > 
> > Why?! Please, try to avoid adding more drivers with this ugly hack.
> 
> My understanding is that this is not a hack, for 38400 we need to pass
> as custom baud. What is the alternative here?

BOTHER. The 38400 is a hack, you lie to the stakeholders that you are at 38.4,
while in real life it means anything.

> I see other drivers are doing this as well, will look into this further
> but not sure if there is a better solution for this.

BOTHER is the solution. Not perfect, but the existing one.

-- 
With Best Regards,
Andy Shevchenko