Messages in this thread | | | Subject | Re: [PATCH] net: macb: fix NULL dereference due to no pcs_config method | From | Nicolas Ferre <> | Date | Thu, 5 Nov 2020 16:22:18 +0100 |
| |
On 05/11/2020 at 15:37, Parshuram Thombare wrote: > This patch fixes NULL pointer dereference due to NULL pcs_config > in pcs_ops. > > Fixes: e4e143e26ce8 ("net: macb: add support for high speed interface")
What is this tag? In linux-next? As patch is not yet in Linus' tree, you cannot refer to it like this.
> Reported-by: Nicolas Ferre <Nicolas.Ferre@microchip.com> > Link: https://lkml.org/lkml/2020/11/4/482
You might need to change this to a "lore" link: https://lore.kernel.org/netdev/2db854c7-9ffb-328a-f346-f68982723d29@microchip.com/
> Signed-off-by: Parshuram Thombare <pthombar@cadence.com>
This fix looks a bit weird to me. What about proposing a patch to Russell like the chunk that you already identified in function phylink_major_config()?
> --- > drivers/net/ethernet/cadence/macb_main.c | 17 +++++++++++++++-- > 1 file changed, 15 insertions(+), 2 deletions(-) > > diff --git a/drivers/net/ethernet/cadence/macb_main.c b/drivers/net/ethernet/cadence/macb_main.c > index b7bc160..130a5af 100644 > --- a/drivers/net/ethernet/cadence/macb_main.c > +++ b/drivers/net/ethernet/cadence/macb_main.c > @@ -633,6 +633,15 @@ static void macb_pcs_an_restart(struct phylink_pcs *pcs) > /* Not supported */ > } > > +static int macb_pcs_config(struct phylink_pcs *pcs, > + unsigned int mode, > + phy_interface_t interface, > + const unsigned long *advertising, > + bool permit_pause_to_mac) > +{ > + return 0; > +}
Russell, is the requirement for this void function intended?
> + > static const struct phylink_pcs_ops macb_phylink_usx_pcs_ops = { > .pcs_get_state = macb_usx_pcs_get_state, > .pcs_config = macb_usx_pcs_config, > @@ -642,6 +651,7 @@ static const struct phylink_pcs_ops macb_phylink_usx_pcs_ops = { > static const struct phylink_pcs_ops macb_phylink_pcs_ops = { > .pcs_get_state = macb_pcs_get_state, > .pcs_an_restart = macb_pcs_an_restart, > + .pcs_config = macb_pcs_config, > }; > > static void macb_mac_config(struct phylink_config *config, unsigned int mode, > @@ -776,10 +786,13 @@ static int macb_mac_prepare(struct phylink_config *config, unsigned int mode, > > if (interface == PHY_INTERFACE_MODE_10GBASER) > bp->phylink_pcs.ops = &macb_phylink_usx_pcs_ops; > - else > + else if (interface == PHY_INTERFACE_MODE_SGMII) > bp->phylink_pcs.ops = &macb_phylink_pcs_ops;
Do you confirm that all SGMII type interfaces need phylink_pcs.ops?
> + else > + bp->phylink_pcs.ops = NULL; > > - phylink_set_pcs(bp->phylink, &bp->phylink_pcs); > + if (bp->phylink_pcs.ops) > + phylink_set_pcs(bp->phylink, &bp->phylink_pcs); > > return 0; > }
Regards, Nicolas
-- Nicolas Ferre
| |