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  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:   Tue, 25 Aug 2020 15:56:15 +0200
From:   Andrew Lunn <andrew@...n.ch>
To:     Kurt Kanzenbach <kurt@...utronix.de>
Cc:     Vivien Didelot <vivien.didelot@...il.com>,
        Florian Fainelli <f.fainelli@...il.com>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
        Rob Herring <robh+dt@...nel.org>, devicetree@...r.kernel.org,
        Sebastian Andrzej Siewior <bigeasy@...utronix.de>,
        Richard Cochran <richardcochran@...il.com>,
        Kamil Alkhouri <kamil.alkhouri@...offenburg.de>,
        ilias.apalodimas@...aro.org, Vladimir Oltean <olteanv@...il.com>
Subject: Re: [PATCH v3 2/8] net: dsa: Add DSA driver for Hirschmann Hellcreek
 switches

> >> +static int hellcreek_detect(struct hellcreek *hellcreek)
> >> +{
> >> +	u16 id, rel_low, rel_high, date_low, date_high, tgd_ver;
> >> +	u8 tgd_maj, tgd_min;
> >> +	u32 rel, date;
> >> +
> >> +	id	  = hellcreek_read(hellcreek, HR_MODID_C);
> >> +	rel_low	  = hellcreek_read(hellcreek, HR_REL_L_C);
> >> +	rel_high  = hellcreek_read(hellcreek, HR_REL_H_C);
> >> +	date_low  = hellcreek_read(hellcreek, HR_BLD_L_C);
> >> +	date_high = hellcreek_read(hellcreek, HR_BLD_H_C);
> >> +	tgd_ver   = hellcreek_read(hellcreek, TR_TGDVER);
> >> +
> >> +	if (id != HELLCREEK_MODULE_ID)
> >> +		return -ENODEV;
> >
> > Are there other Hellcreek devices? I'm just wondering if we should
> > have a specific compatible for 0x4c30 as well as the more generic 
> > "hirschmann,hellcreek".
> 
> Yes, there will be different revisions of the Hellcreek devices. This ID
> is really device specific. A lot of features of this switch are
> configured in the VHDL code. For instance the MAC settings (100 or 1000
> Mbit/s).
> 
> I've discussed this with the HW engineer from Hirschmann. He suggested
> to keep this check here, as the driver is currently specific for the
> that device. We have to make sure that the driver matches the hardware.

I agree with the check here. The question is about the compatible
string. Should there be a more specific compatible string as well as
the generic one?

There have been a few discussions about how the Marvell DSA driver
does its compatible string. The compatible string tells you where to
find the ID register, not what value to expect in the ID register. The
ID register can currently be in one of three different locations. Do
all current and future Hellcreak devices have the same value for
HR_MODID_C? If not, now is a good time to add a more specific
compatible string to tell you where to find the ID register.

> My plan was to extend this when I have access to other revisions. There
> will be a SPI variant as well. But, I didn't want to implement it without the
> ability to test it.

Does the SPI variant use the same value for HR_MODID_C? Maybe you need
a different compatible, maybe not, depending on how the driver is
structured.

The compatible string is part of the ABI. So thinking about it a bit
now can make things easier later. I just want to make sure you have
thought about this.

	Andrew

Powered by blists - more mailing lists