linux-kernel - Re: [PATCH v4 0/4] add device drivers for Siemens Industrial PCs

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: <20211126182823.5046ccce@md1za8fc.ad001.siemens.net>
Date:   Fri, 26 Nov 2021 18:28:23 +0100
From:   Henning Schild <henning.schild@...mens.com>
To:     Guenter Roeck <linux@...ck-us.net>
Cc:     linux-kernel@...r.kernel.org, linux-leds@...r.kernel.org,
        platform-driver-x86@...r.kernel.org,
        linux-watchdog@...r.kernel.org,
        Srikanth Krishnakar <skrishnakar@...il.com>,
        Jan Kiszka <jan.kiszka@...mens.com>,
        Gerd Haeussler <gerd.haeussler.ext@...mens.com>,
        Wim Van Sebroeck <wim@...ux-watchdog.org>,
        Mark Gross <mgross@...ux.intel.com>,
        Hans de Goede <hdegoede@...hat.com>,
        Pavel Machek <pavel@....cz>,
        Andy Shevchenko <andy.shevchenko@...il.com>,
        Enrico Weigelt <lkml@...ux.net>
Subject: Re: [PATCH v4 0/4] add device drivers for Siemens Industrial PCs

Am Fri, 26 Nov 2021 08:02:11 -0800
schrieb Guenter Roeck <linux@...ck-us.net>:

> On 11/26/21 7:34 AM, Henning Schild wrote:
> > Hi all,
> > 
> > in p3 not too much was left open so i hope this might be the last
> > and might be quick.
> > 
> > The two points that have been open where:
> >   1 wish to split wdt driver into two
> >   2 wish to use pinctrl for LEDs/WDT
> > 
> > 1 was ignored for now. We can split later when we add more devices.
> > It remains unclear if splitting is the way to go when more devices
> > come.  
> 
> The code is already quite messy, in part because memory regions are
> declared locally and not passed through the parent device as they
> should.

Oh is it, can you please be more precise when being so "harsh". The
parent driver is just a detector which devices in which variant are
available on a given box. It does not drive and should not claim
resources i guess. 

> I don't see how splitting the driver into multiple drivers
> would improve the situation.

In fact that already is a split i would say.

> The platform code claims to be inspired by the lpc_ich driver. 

Only the P2SB bit, getting the memory base of the pinctrl memory.

> Maybe it should take a real example from that
> and pass version or variant specific details through platform data
> instead of maintaining it in the watchdog driver.

It does that ... or i misunderstand you. The driver from p1 can find
these three watchdog types
SIMATIC_IPC_DEVICE_NONE
SIMATIC_IPC_DEVICE_227E
SIMATIC_IPC_DEVICE_427E

Which one to use is determined in the platform driver that will 
platform_device_register_data(NULL, KBUILD_MODNAME "_wdt" ...
&platform_data)

So we tell the watchdog driver about a watchdog and tell it which one
...

If we ever split we could 
   register(KBUILD_MODNAME "_wdt_227e"
 or 
   register(KBUILD_MODNAME "_wdt_427e"

with no platform data needed anymore.

But as i said, i do not want to split just yet. Just want to defend
that code when someone calls it "pretty messy" so late in the process.

Henning

> Guenter