linux-kernel - Re: [PATCH v2 3/4] Watchdog: sp5100_tco: Add EFCH SMBus controller initialization using MMIO

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]

Message-ID: <YdgeU2p+i5hN1XCU@rric.localdomain>
Date:   Fri, 7 Jan 2022 12:05:45 +0100
From:   Robert Richter <rrichter@....com>
To:     Terry Bowman <Terry.Bowman@....com>
CC:     Guenter Roeck <linux@...ck-us.net>,
        <linux-watchdog@...r.kernel.org>, <linux-kernel@...r.kernel.org>,
        <wim@...ux-watchdog.org>, <ssg.sos.patches@....com>,
        <sudheesh.mavila@....com>,
        "Lendacky, Thomas" <thomas.lendacky@....com>
Subject: Re: [PATCH v2 3/4] Watchdog: sp5100_tco: Add EFCH SMBus controller
 initialization using MMIO

On 06.01.22 13:07:11, Terry Bowman wrote:
> On 1/6/22 12:18 PM, Guenter Roeck wrote:
> > On Wed, Nov 03, 2021 at 11:15:20AM -0500, Terry Bowman wrote:

> >> diff --git a/drivers/watchdog/sp5100_tco.c b/drivers/watchdog/sp5100_tco.c
> >> index 80ae42ae7aaa..4777e672a8ad 100644
> >> --- a/drivers/watchdog/sp5100_tco.c
> >> +++ b/drivers/watchdog/sp5100_tco.c
> >> @@ -48,12 +48,14 @@
> >>  /* internal variables */
> >>  
> >>  enum tco_reg_layout {
> >> -	sp5100, sb800, efch
> >> +	sp5100, sb800, efch, efch_mmio
> >>  };
> >>  
> >>  struct sp5100_tco {
> >>  	struct watchdog_device wdd;
> >>  	void __iomem *tcobase;
> >> +	void __iomem *addr;
> >> +	struct resource *res;
> > 
> > I must admit that I really don't like this code. Both res and
> > addr are only used during initialization, yet their presence suggests
> > runtime usage. Any chance to reqork this to not require those variables ?

We did that in an earlier version, see struct efch_cfg of:

 https://patchwork.kernel.org/project/linux-watchdog/patch/20210813213216.54780-1-Terry.Bowman@amd.com/

The motivation of it was the same as you suggested to only use it
during init.

Having it in struct sp5100_tco made things simpler esp. in the
definition of the function interfaces where those new members are
used.

If that init vars are no longer in struct sp5100_tco then callers of
efch_read_pm_reg8() and efch_update_pm_reg8() will need to carry a
pointer to them. To avoid this I see those options:

1) Implement them as global (or a single global struct) and possibly
protect it by a mutex. There is only a single device anyway and we
wouldn't need a protection.

2) Have an own mmio implementation of tco_timer_enable() and/or
sp5100_tco_timer_init().

> Yes, v3 will include refactoring to remove 'res' and 'addr'. I will also 
> correct the trailing newline you mentioned in an earlier email.
> 
> Regards,
> Terry
> 
> >>  	enum tco_reg_layout tco_reg_layout;

While at it, tco_reg_layout is also only used during initialization
and can be moved there too. This would raise option 3:

3) Add a pointer of struct sp5100_tco to struct efch_cfg and use that
struct instead in init funtions only. But that causes the most rework
(which would be ok to me).

Going with 3) looks the cleanest way, I would try that. But all
options have its advantages.

-Robert

> >>  };