Messages in this thread | | | Date | Tue, 7 Mar 2023 11:16:11 -0500 | Subject | Re: [PATCH net-next] net: mdio: Add netlink interface | From | Sean Anderson <> |
| |
On 3/7/23 09:22, Andrew Lunn wrote: >> To prevent userspace phy drivers, writes are disabled by default, and can >> only be enabled by editing the source. This is the same strategy used by >> regmap for debugfs writes. Unfortunately, this disallows several useful >> features, including >> >> - Register writes (obviously) >> - C45-over-C22 > > You could add C45-over-C22 as another op. > > This tool is dangerous, even in its read only mode, just like the > IOCTL interface. Interrupt status registers are often clear on read, > so you can loose interrupts. Statistics counters are sometimes clear > on read. BMSR link bit is also latching, so a read of it could mean > you miss link events, etc. Adding C45-over-C22 is just as dangerous, > you can mess up MDIO switches which use the registers for other > things, but by deciding to use this tool you have decided to take the > risk of blowing your foot off.
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. Tainting on read would be fine, since it is certainly possible to imagine hardware which would malfunction on read. I suppose regmap gets away with this by sticking the whole thing in debugfs.
>> - Atomic access to paged registers >> - Better MDIO emulation for e.g. QEMU >> >> However, the read-only interface remains broadly useful for debugging. > > I would say it is broadly useful for PHYs. But not Ethernet switches, > when in read only mode. > >> +static int mdio_nl_open(struct mdio_nl_xfer *xfer); >> +static int mdio_nl_close(struct mdio_nl_xfer *xfer, bool last, int xerr); > > I guess i never did a proper review of this code before, due to not > liking the concept.... > > Move the code around so these are not needed, unless there are > functions which are mutually recursive.
They do indeed call each other (although by my analysis no recursion results). The forward declaration of mdio_nl_open is unnecessary, so I will rearrange things to avoid that.
>> +static inline u16 *__arg_r(u32 arg, u16 *regs) >> +{ >> + WARN_ON_ONCE(arg >> 16 != MDIO_NL_ARG_REG); >> + >> + return ®s[arg & 0x7]; >> +} > > No inline functions in C files. Leave the compiler to decide.
OK
>> +static int mdio_nl_eval(struct mdio_nl_xfer *xfer) >> +{ >> + struct mdio_nl_insn *insn; >> + unsigned long timeout; >> + u16 regs[8] = { 0 }; >> + int pc, ret = 0; >> + int phy_id, reg, prtad, devad, val; >> + >> + timeout = jiffies + msecs_to_jiffies(xfer->timeout_ms); >> + >> + mutex_lock(&xfer->mdio->mdio_lock); > > Should timeout be set inside the lock, for when you have two > applications running in parallel and each take a while?
That seems reasonable.
>> + >> + 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.
> As you said in > the cover note, the ioctl interface is limiting when there is no PHY, > so you are artificially adding the same restriction here.
I don't follow.
> Also, you > might want to do C45 on a C22 PHY, e.g. to access EEE registers. Plus > you could consider adding C45 over C22 here.
As Russell noted, this is dangerous in the general case.
>> + >> + if (ret < 0) >> + goto exit; >> + *__arg_r(insn->arg2, regs) = ret; >> + ret = 0; >> + break; >> + >> + case MDIO_NL_OP_WRITE: >> + 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); >> + val = __arg_ri(insn->arg2, regs); >> + >> +#ifdef MDIO_NETLINK_ALLOW_WRITE >> + add_taint(TAINT_USER, LOCKDEP_STILL_OK); > > I don't know, but maybe taint on read as well. > >> + 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.
--Sean
| |