linux-kernel - Re: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO configuration

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] [thread-next>] [day] [month] [year] [list]

Date:   Sun, 4 Jun 2023 20:29:58 +0200
From:   Greg KH <gregkh@...uxfoundation.org>
To:     Hugo Villeneuve <hugo@...ovil.com>
Cc:     robh+dt@...nel.org, krzysztof.kozlowski+dt@...aro.org,
        conor+dt@...nel.org, jirislaby@...nel.org, jringle@...dpoint.com,
        tomasz.mon@...lingroup.com, l.perczak@...lintechnologies.com,
        linux-serial@...r.kernel.org, devicetree@...r.kernel.org,
        linux-kernel@...r.kernel.org, linux-gpio@...r.kernel.org,
        Hugo Villeneuve <hvilleneuve@...onoff.com>,
        stable@...r.kernel.org, Andy Shevchenko <andy.shevchenko@...il.com>
Subject: Re: [PATCH v7 5/9] serial: sc16is7xx: fix regression with GPIO
 configuration

On Sun, Jun 04, 2023 at 01:43:44PM -0400, Hugo Villeneuve wrote:
> Here is what I suggest to silence the warning:
> 
> 	mctrl_mask = sc16is7xx_setup_mctrl_ports(dev);
> 
> #ifdef CONFIG_GPIOLIB
> 	ret = sc16is7xx_setup_gpio_chip(dev, mctrl_mask);
> 	if (ret)
> 		goto out_thread;
> #else
> 	(void) mctrl_mask;
> #endif

Eeek,  no, please no...

First off, please don't put #ifdef in .c files if at all possible.
Secondly, that (void) craziness is just that.  Rework this to not be an
issue some other way please.

> I could also store (define new variable) mctrl_mask directly inside struct sc16is7xx_port...

Sure, that sounds best.

> > And you have a real port here, no need to pass in a "raw" struct device,
> > right?
> 
> The function operates globally on both ports (or nr_uart), not just a single port. That is why I pass the "raw" struct device, in order to extract the 
> struct sc16is7xx_port from it:
> 
>     struct sc16is7xx_port *s = dev_get_drvdata(dev);
> 
> Inside the function, I also need the "raw" struc device. If we pass a struct sc16is7xx_port to the function, then I can get the "raw" struc device with this:
> 
> static u8 sc16is7xx_setup_mctrl_ports(struct sc16is7xx_port *s)
> {
> 	struct device *dev = &s->p[0].port.dev;
> 
> But I find this more obfuscated and hard to understand than to simply pass a "raw" struct device...

You should never need a "raw" struct device for stuff (if so, something
is really odd).  Except for error messages, but that's not really a big
deal, right?

Don't pass around struct device in a driver, use the real types as you
know you have it and it saves odd casting around and it just doesn't
look safe at all to do so.

And if you have that crazy s->p.... stuff in multiple places, then
perhaps you might want to rethink the structure somehow?  Or at the very
least, write an inline function to get it when needed.

Also, meta comment, you might want to use some \n characters in your
emails, your lines are really long :)

thanks,

greg k-h