Messages in this thread | | | Date | Tue, 7 Mar 2023 12:42:50 -0500 | Subject | Re: [PATCH net-next] net: mdio: Add netlink interface | From | Sean Anderson <> |
| |
On 3/7/23 12:23, Andrew Lunn wrote: >> Yes, and I should probably have commented on this in the commit message. >> IMO the things you listed are... iffy but unlikely to cause a >> malfunction. > > You consider a missed interrupt not a malfunction?
Hm, yeah that would probably do it.
>> >> + >> >> + for (insn = xfer->prog, pc = 0; >> >> + pc < xfer->prog_len; >> >> + insn = &xfer->prog[++pc]) { >> >> + if (time_after(jiffies, timeout)) { >> >> + ret = -ETIMEDOUT; >> >> + break; >> >> + } >> >> + >> >> + switch ((enum mdio_nl_op)insn->op) { >> >> + case MDIO_NL_OP_READ: >> >> + phy_id = __arg_ri(insn->arg0, regs); >> >> + prtad = mdio_phy_id_prtad(phy_id); >> >> + devad = mdio_phy_id_devad(phy_id); >> >> + reg = __arg_ri(insn->arg1, regs); >> >> + >> >> + if (mdio_phy_id_is_c45(phy_id)) >> >> + ret = __mdiobus_c45_read(xfer->mdio, prtad, >> >> + devad, reg); >> >> + else >> >> + ret = __mdiobus_read(xfer->mdio, phy_id, reg); >> > >> > The application should say if it want to do C22 or C45. >> >> The phy_id comes from the application. So it sets MDIO_PHY_ID_C45 if it wants >> to use C45. > > Ah, i misunderstood what mdio_phy_id_is_c45() does. > > Anyway, i don't like MDIO_PHY_ID_C45, it has been pretty much removed > everywhere with the refactoring of MDIO drivers to export read and > read_c45 etc. PHY drivers also don't use it, they use c22 or c45 > specific methods. So i would prefer an additional attribute. That also > opens up the possibility of adding C45 over C22.
Well, this is really just because there is an existing way to specify c22 and c45 addresses in a u16. We could definitely add a "please do C45 over C22" flag. That said, I think that sort of thing is handled better by allowing writes in the general case.
>> As Russell noted, this is dangerous in the general case. > > And Russell also agreed this whole module is dangerous in general. > Once you accept it is dangerous, its a debug tool only, why not allow > playing with a bit more fire? You could at least poke around the MDIO > bus structures and see if a PHY has been found, and it not, block C45 > over C22.
I can look into that.
>> >> + if (mdio_phy_id_is_c45(phy_id)) >> >> + ret = __mdiobus_c45_write(xfer->mdio, prtad, >> >> + devad, reg, val >> >> + else >> >> + ret = __mdiobus_write(xfer->mdio, dev, reg, >> >> + val); >> >> +#else >> >> + ret = -EPERM; >> > >> > EPERM is odd, EOPNOTSUPP would be better. EPERM suggests you can run >> > it as root and it should work. >> >> Well, EPERM is what you get when trying to write a 444 file, which is >> effectively what we're enforcing here. > > Does it change to 644 when write is enabled?
Yes. But it is more like 400 and 600.
> But netlink does not even use file access permissions.
Is EPERM reserved only for files?
> I would probably trap this earlier, where you have a extack instance > you can return a meaningful error message string.
That sounds good.
--Sean
| |