| 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
| ||
|
Message-ID: <CA+V-a8vLfwr7gKruY-SOdBHUPmY5TgQ3vrSMkn+QF7LsphVxuA@mail.gmail.com>
Date: Mon, 12 Jan 2026 12:12:05 +0000
From: "Lad, Prabhakar" <prabhakar.csengg@...il.com>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Clément Léger <clement.leger@...tlin.com>,
Andrew Lunn <andrew+netdev@...n.ch>, "David S. Miller" <davem@...emloft.net>,
Eric Dumazet <edumazet@...gle.com>, Jakub Kicinski <kuba@...nel.org>, Paolo Abeni <pabeni@...hat.com>,
Rob Herring <robh@...nel.org>, Krzysztof Kozlowski <krzk+dt@...nel.org>, Conor Dooley <conor+dt@...nel.org>,
Heiner Kallweit <hkallweit1@...il.com>, Geert Uytterhoeven <geert+renesas@...der.be>,
Magnus Damm <magnus.damm@...il.com>, linux-renesas-soc@...r.kernel.org,
netdev@...r.kernel.org, devicetree@...r.kernel.org,
linux-kernel@...r.kernel.org, Biju Das <biju.das.jz@...renesas.com>,
Fabrizio Castro <fabrizio.castro.jz@...esas.com>,
Lad Prabhakar <prabhakar.mahadev-lad.rj@...renesas.com>
Subject: Re: [PATCH net-next v2 2/2] net: pcs: rzn1-miic: Add support for PHY
link active-level configuration
Hi Russell,
Thank you for the review.
On Fri, Jan 9, 2026 at 3:51 PM Russell King (Oracle)
<linux@...linux.org.uk> wrote:
>
> On Fri, Jan 09, 2026 at 02:22:50PM +0000, Prabhakar wrote:
> > +static void miic_configure_phylink(struct miic *miic, u32 conf,
> > + u32 port, bool active_low)
> > +{
> > + bool polarity_active_high;
> > + u32 mask, val;
> > + int shift;
> > +
> > + /* determine shift and polarity for this conf */
> > + if (miic->of_data->type == MIIC_TYPE_RZN1) {
> > + switch (conf) {
> > + /* switch ports => bits [3:0] (shift 0), active when low */
> > + case MIIC_SWITCH_PORTA:
> > + case MIIC_SWITCH_PORTB:
> > + case MIIC_SWITCH_PORTC:
> > + case MIIC_SWITCH_PORTD:
> > + shift = 0;
> > + polarity_active_high = false;
> > + break;
> > +
> > + /* EtherCAT ports => bits [7:4] (shift 4), active when high */
> > + case MIIC_ETHERCAT_PORTA:
> > + case MIIC_ETHERCAT_PORTB:
> > + case MIIC_ETHERCAT_PORTC:
> > + shift = 4;
> > + polarity_active_high = true;
> > + break;
> > +
> > + /* Sercos ports => bits [11:8] (shift 8), active when high */
> > + case MIIC_SERCOS_PORTA:
> > + case MIIC_SERCOS_PORTB:
> > + shift = 8;
> > + polarity_active_high = true;
> > + break;
> > +
> > + default:
> > + return;
> > + }
> > + } else {
> > + switch (conf) {
> > + /* ETHSW ports => bits [3:0] (shift 0), active when low */
> > + case ETHSS_ETHSW_PORT0:
> > + case ETHSS_ETHSW_PORT1:
> > + case ETHSS_ETHSW_PORT2:
> > + shift = 0;
> > + polarity_active_high = false;
> > + break;
> > +
> > + /* ESC ports => bits [7:4] (shift 4), active when high */
> > + case ETHSS_ESC_PORT0:
> > + case ETHSS_ESC_PORT1:
> > + case ETHSS_ESC_PORT2:
> > + shift = 4;
> > + polarity_active_high = true;
> > + break;
> > +
> > + default:
> > + return;
> > + }
> > + }
> > +
> > + mask = BIT(port) << shift;
> > +
> > + if (polarity_active_high)
> > + val = (active_low ? 0 : BIT(port)) << shift;
> > + else
> > + val = (active_low ? BIT(port) : 0) << shift;
>
> Looking closer at this, I think this is confusing.
>
> The underlying purpose here is to set mask and val to change the state of
> a single bit in the PHY link register for each call to this function,
> accumulating the changes in your misnamed "struct phylink".
>
> Given that "mask" can be used to compute the value to describe the bit,
> and that is made up of "shift" that describes the start of the bitfield
> and "port" that describes the bit within the bitfield, then surely:
>
> mask = BIT(port + shift);
>
> would be saner?
>
Agreed.
> Next, the creation of "val". This is either zero or the same value of
> mask depending on active_low and polarity_active_high. The truth table
> here is:
>
> polarity_active_high active_low result
> 0 0 0
> 0 1 mask
> 1 0 mask
> 1 1 0
>
> This is a classical an exclusive-or truth table in the world of logic,
> or could be regarded as an inquality relationship (result is mask
> when polarity_active_high differs from active_low, otherwise zero).
>
> Thus:
>
> /* Set the bit when polarity_active_high differs from active_low */
> val = polarity_active_high != active_low ? mask : 0;
>
> Or, even simpler, this could become overall:
>
> mask = BIT(port + shift);
>
> miic->phylink.mask |= mask;
> if (polarity_active_high != active_low)
> miic->phylink.val |= mask;
> else
> miic->phylink.val &= ~mask;
>
Ack, this simplifies the code. I’ll rework the code along the lines
you suggested.
> > @@ -605,8 +698,15 @@ static int miic_parse_dt(struct miic *miic, u32 *mode_cfg)
> >
> > /* Adjust for 0 based index */
> > port += !miic->of_data->miic_port_start;
> > - if (of_property_read_u32(conv, "renesas,miic-input", &conf) == 0)
> > - dt_val[port] = conf;
> > + if (of_property_read_u32(conv, "renesas,miic-input", &conf))
> > + continue;
> > +
> > + dt_val[port] = conf;
> > +
> > + active_low = of_property_read_bool(conv, "renesas,miic-phylink-active-low");
> > +
> > + miic_configure_phylink(miic, conf, port - !miic->of_data->miic_port_start,
> > + active_low);
>
> I think this is also over-complicated. Wouldn't it be better to only
> deal with the miic_port_start at the one place that it matters?
>
> if (of_property_read_u32(conv, "reg", &port))
> continue;
>
> if (of_property_read_u32(conv, "renesas,miic-input", &conf))
> continue;
>
> dt_val[port + !miic->of_data->miic_port_start] = conf;
>
> active_low = of_property_read_bool(conv, "renesas,miic-phylink-active-low");
>
> miic_configure_phylink(miic, conf, port, active_low);
>
> ?
>
Agreed with your point about miic_port_start. Handling the offset only
at the point where dt_val[] is indexed, and passing the unmodified
port into miic_configure_phylink(), simplifies the flow and avoids the
current back-and-forth adjustments. I’ll restructure that part
accordingly.
I’ll post an updated version with these cleanups in the next revision.
Cheers,
Prabhakar
Powered by blists - more mailing lists