linux-kernel - RE: [PATCH v2] serial: samsung: honor fifosize from dts at first

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: <006b01da58f0$601316b0$20394410$@samsung.com>
Date: Tue, 6 Feb 2024 17:03:56 +0530
From: "Tamseel Shams" <m.shams@...sung.com>
To: "'Krzysztof Kozlowski'" <krzysztof.kozlowski@...aro.org>,
	<alim.akhtar@...sung.com>, <krzysztof.kozlowski+dt@...aro.org>,
	<gregkh@...uxfoundation.org>, <jirislaby@...nel.org>
Cc: <linux-arm-kernel@...ts.infradead.org>,
	<linux-samsung-soc@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
	<linux-serial@...r.kernel.org>
Subject: RE: [PATCH v2] serial: samsung: honor fifosize from dts at first

Hi Krzysztof,

> -----Original Message-----
> From: Krzysztof Kozlowski [mailto:krzysztof.kozlowski@...aro.org]
> Sent: 06 February 2024 15:42
> To: Tamseel Shams <m.shams@...sung.com>; alim.akhtar@...sung.com;
> krzysztof.kozlowski+dt@...aro.org; gregkh@...uxfoundation.org;
> jirislaby@...nel.org
> Cc: linux-arm-kernel@...ts.infradead.org; linux-samsung-
> soc@...r.kernel.org; linux-kernel@...r.kernel.org; linux-
> serial@...r.kernel.org
> Subject: Re: [PATCH v2] serial: samsung: honor fifosize from dts at first
> 
> On 05/02/2024 09:24, Tamseel Shams wrote:
> > Currently for platforms which passes UART fifosize from DT gets
> > override by local driver structure "s3c24xx_serial_drv_data", which is
> > not intended. Change the code to honor fifosize from device tree at
> > first.
> >
> > Signed-off-by: Tamseel Shams <m.shams@...sung.com>
> > ---
> > Change Log:
> > v1 -> v2:
> > Acknowledged Krzysztof's comments
> > Initialized "ret" variable
> >
> >  drivers/tty/serial/samsung_tty.c | 16 +++++++++-------
> >  1 file changed, 9 insertions(+), 7 deletions(-)
> >
> > diff --git a/drivers/tty/serial/samsung_tty.c
> > b/drivers/tty/serial/samsung_tty.c
> > index 71d17d804fda..e5dc2c32b1bd 100644
> > --- a/drivers/tty/serial/samsung_tty.c
> > +++ b/drivers/tty/serial/samsung_tty.c
> > @@ -1952,7 +1952,7 @@ static int s3c24xx_serial_probe(struct
> platform_device *pdev)
> >  	struct device_node *np = pdev->dev.of_node;
> >  	struct s3c24xx_uart_port *ourport;
> >  	int index = probe_index;
> > -	int ret, prop = 0;
> > +	int ret = 1, prop = 0;
> 
> I am sorry, but return of probe function cannot be positive.
> 

Thanks for the review.

I am reusing the "ret" variable to check whether fifosize property
is present in DT or not. It will be overwritten at later stage in probe
function before returning. Also, "ret" variable is used to return from
probe function only in case of failure i.e. "ret" is negative.
Currently, probe function always returns 0 in case of success.

For better readability, I am thinking of introducing a new variable for
checking the return value from function "of_property_read_u32" while
getting "samsung,uart-fifosize" property.

What are your thoughts on that?

> >
> >  	if (np) {
> >  		ret = of_alias_get_id(np, "serial"); @@ -1990,8 +1990,7 @@
> static
> > int s3c24xx_serial_probe(struct platform_device *pdev)
> >  	}
> >
> >  	if (np) {
> > -		of_property_read_u32(np,
> > -			"samsung,uart-fifosize", &ourport->port.fifosize);
> > +		ret = of_property_read_u32(np, "samsung,uart-fifosize",
> > +&ourport->port.fifosize);
> >
> >  		if (of_property_read_u32(np, "reg-io-width", &prop) == 0) {
> >  			switch (prop) {
> > @@ -2009,10 +2008,13 @@ static int s3c24xx_serial_probe(struct
> platform_device *pdev)
> >  		}
> >  	}
> >
> > -	if (ourport->drv_data->fifosize[index])
> > -		ourport->port.fifosize = ourport->drv_data->fifosize[index];
> > -	else if (ourport->info->fifosize)
> > -		ourport->port.fifosize = ourport->info->fifosize;
> > +	if (ret) {
> 
> Are you sure that you are checking correct ret?

Explanation same as above. 


Thanks & Regards,
Tamseel Shams