linux-kernel - Re: [PATCH 20/22] watchdog: jz4740_wdt: Use 'dev' instead of dereferencing it repeatedly

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 for Android: free password hash cracker in your pocket

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

Message-ID: <20190410174746.GA2132@roeck-us.net>
Date:   Wed, 10 Apr 2019 10:47:46 -0700
From:   Guenter Roeck <linux@...ck-us.net>
To:     Joe Perches <joe@...ches.com>
Cc:     Wim Van Sebroeck <wim@...ux-watchdog.org>,
        linux-watchdog@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 20/22] watchdog: jz4740_wdt: Use 'dev' instead of
 dereferencing it repeatedly

Hi Joe,

On Wed, Apr 10, 2019 at 09:52:09AM -0700, Joe Perches wrote:
> On Wed, 2019-04-10 at 09:28 -0700, Guenter Roeck wrote:
> > Introduce local variable 'struct device *dev' and use it instead of
> > dereferencing it repeatedly. Also, there is no call to dev_get_drvdata()
> > or platform_get_drvdata() in the driver, so drop the unnecessary
> > call to platform_set_drvdata().
> 
> Dropping platform_set_drvdata seems to me like it should
> be a separate patch.
> 

Bundling all changes into one patch per driver already resulted in more than
60 patches total in this series. Splitting that into three sets of patches
over three days already earned me automated replies telling me that I am now
considered to be a spammer. One logical change per patch would have resulted
in hundreds of patches. I don't think that would have scaled well.

I considered other splits, such as one coccinelle rule per patch, affecting
multiple drivers, but that would have had the same result since it would have
needed dozens of Cc: lines per patch. Ultimately, I decided to go with one patch
per file.

> And are you sure no other function uses a get_drvdata call?
> Maybe something in watchdog_dev.c?  Possibly:
> 
>    #ifdef CONFIG_WATCHDOG_SYSFS
>    static ssize_t nowayout_show(struct device *dev, struct device_attribute *attr,
>    				char *buf)
>    {
>    	struct watchdog_device *wdd = dev_get_drvdata(dev);
> 

'dev' in nowayout_show() points to the watchdog device, not to the platform
device. Its drvdata is set in drivers/base/core.c:device_create_groups_vargs().
Not all watchdog drivers are platform drivers, and the watchdog core can not
depend on a watchdog device even having a parent device, much less make
assumptions about its drvdata.

Guenter