| 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: <YhOeftlyzP0U9zR8@shell.armlinux.org.uk>
Date: Mon, 21 Feb 2022 14:15:26 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Vladimir Oltean <olteanv@...il.com>
Cc: Ansuel Smith <ansuelsmth@...il.com>, Andrew Lunn <andrew@...n.ch>,
"David S. Miller" <davem@...emloft.net>,
Florian Fainelli <f.fainelli@...il.com>,
Jakub Kicinski <kuba@...nel.org>, netdev@...r.kernel.org,
Vivien Didelot <vivien.didelot@...il.com>
Subject: Re: [PATCH net-next v2 1/6] net: dsa: add support for phylink
mac_select_pcs()
On Mon, Feb 21, 2022 at 01:30:09PM +0000, Russell King (Oracle) wrote:
> On Sat, Feb 19, 2022 at 11:22:24PM +0200, Vladimir Oltean wrote:
> > On Sat, Feb 19, 2022 at 11:12:41PM +0200, Vladimir Oltean wrote:
> > > > static const struct phylink_mac_ops dsa_port_phylink_mac_ops = {
> > > > .validate = dsa_port_phylink_validate,
> > > > + .mac_select_pcs = dsa_port_phylink_mac_select_pcs,
> > >
> > > This patch breaks probing on DSA switch drivers that weren't converted
> > > to supported_interfaces, due to this check in phylink_create():
> >
> > And this is only the most superficial layer of breakage. Everywhere in
> > phylink.c where pl->mac_ops->mac_select_pcs() is used, its presence is
> > checked and non-zero return codes from it are treated as hard errors,
> > even -EOPNOTSUPP, even if this particular error code is probably
> > intended to behave identically as the absence of the function pointer,
> > for compatibility.
>
> I don't understand what problem you're getting at here - and I don't
> think there is a problem.
>
> While I know it's conventional in DSA to use EOPNOTSUPP to indicate
> that a called method is not implemented, this is not something that
> is common across the board - and is not necessary here.
>
> The implementation of dsa_port_phylink_mac_select_pcs() returns a
> NULL PCS when the DSA operation for it is not implemented. This
> means that:
>
> 1) phylink_validate_mac_and_pcs() won't fail due to mac_select_pcs()
> being present but DSA drivers not implementing it.
>
> 2) phylink_major_config() will not attempt to call phylink_set_pcs()
> to change the PCS.
>
> So, that much is perfectly safe.
>
> As for your previous email reporting the problem with phylink_create(),
> thanks for the report and sorry for the breakage - the breakage was
> obviously not intended, and came about because of all the patch
> shuffling I've done over the last six months trying to get these
> changes in, and having forgotten about this dependency.
>
> I imagine the reason you've raised EOPNOTSUPP is because you wanted to
> change dsa_port_phylink_mac_select_pcs() to return an error-pointer
> encoded with that error code rather than NULL, but you then (no
> surprises to me) caused phylink to fail.
>
> Considering the idea of using EOPNOTSUPP, at the two places we call
> mac_select_pcs(), we would need to treat this the same way we currently
> treat NULL. We would also need phylink_create() to call
> mac_select_pcs() if the method is non-NULL to discover if the DSA
> sub-driver implements the method - but we would need to choose an
> interface at this point.
>
> I think at this point, I'd rather:
>
> 1) add a bool in struct phylink to indicate whether we should be calling
> mac_select_pcs, and replace the
>
> if (pl->mac_ops->mac_select_pcs)
>
> with
>
> if (pl->using_mac_select_pcs)
>
> 2) have phylink_create() do:
>
> bool using_mac_select_pcs = false;
>
> if (mac_ops->mac_select_pcs &&
> mac_ops->mac_select_pcs(config, PHY_INTERFACE_MODE_NA) !=
> ERR_PTR(-EOPNOTSUPP))
> using_mac_select_pcs = true;
>
> if (using_mac_select_pcs &&
> phy_interface_empty(config->supported_interfaces)) {
> ...
>
> ...
>
> pl->using_mac_select_pcs = using_mac_select_pcs;
>
> which should give what was intended until DSA drivers are all updated
> to fill in config->supported_interfaces.
Please try this patch, thanks:
diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 6c7ab4a7a3be..de0557bbd4a7 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -74,6 +74,7 @@ struct phylink {
struct work_struct resolve;
bool mac_link_dropped;
+ bool using_mac_select_pcs;
struct sfp_bus *sfp_bus;
bool sfp_may_have_phy;
@@ -416,7 +417,7 @@ static int phylink_validate_mac_and_pcs(struct phylink *pl,
int ret;
/* Get the PCS for this interface mode */
- if (pl->mac_ops->mac_select_pcs) {
+ if (pl->using_mac_select_pcs) {
pcs = pl->mac_ops->mac_select_pcs(pl->config, state->interface);
if (IS_ERR(pcs))
return PTR_ERR(pcs);
@@ -791,7 +792,7 @@ static void phylink_major_config(struct phylink *pl, bool restart,
phylink_dbg(pl, "major config %s\n", phy_modes(state->interface));
- if (pl->mac_ops->mac_select_pcs) {
+ if (pl->using_mac_select_pcs) {
pcs = pl->mac_ops->mac_select_pcs(pl->config, state->interface);
if (IS_ERR(pcs)) {
phylink_err(pl,
@@ -1204,11 +1205,17 @@ struct phylink *phylink_create(struct phylink_config *config,
phy_interface_t iface,
const struct phylink_mac_ops *mac_ops)
{
+ bool using_mac_select_pcs = false;
struct phylink *pl;
int ret;
- /* Validate the supplied configuration */
if (mac_ops->mac_select_pcs &&
+ mac_ops->mac_select_pcs(config, PHY_INTERFACE_MODE_NA) !=
+ ERR_PTR(-EOPNOTSUPP))
+ using_mac_select_pcs = true;
+
+ /* Validate the supplied configuration */
+ if (using_mac_select_pcs &&
phy_interface_empty(config->supported_interfaces)) {
dev_err(config->dev,
"phylink: error: empty supported_interfaces but mac_select_pcs() method present\n");
@@ -1232,6 +1239,7 @@ struct phylink *phylink_create(struct phylink_config *config,
return ERR_PTR(-EINVAL);
}
+ pl->using_mac_select_pcs = using_mac_select_pcs;
pl->phy_state.interface = iface;
pl->link_interface = iface;
if (iface == PHY_INTERFACE_MODE_MOCA)
diff --git a/net/dsa/port.c b/net/dsa/port.c
index 258782bf4271..367d141c6971 100644
--- a/net/dsa/port.c
+++ b/net/dsa/port.c
@@ -1058,8 +1058,8 @@ dsa_port_phylink_mac_select_pcs(struct phylink_config *config,
phy_interface_t interface)
{
struct dsa_port *dp = container_of(config, struct dsa_port, pl_config);
+ struct phylink_pcs *pcs = ERR_PTR(-EOPNOTSUPP);
struct dsa_switch *ds = dp->ds;
- struct phylink_pcs *pcs = NULL;
if (ds->ops->phylink_mac_select_pcs)
pcs = ds->ops->phylink_mac_select_pcs(ds, dp->index, interface);
--
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Powered by blists - more mailing lists