lkml.org 
[lkml]   [2023]   [Mar]   [7]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
    /
    Date
    From
    SubjectRe: [PATCH net-next] net: mdio: Add netlink interface
    > 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.

    > - 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.

    > +static inline u16 *__arg_r(u32 arg, u16 *regs)
    > +{
    > + WARN_ON_ONCE(arg >> 16 != MDIO_NL_ARG_REG);
    > +
    > + return &regs[arg & 0x7];
    > +}

    No inline functions in C files. Leave the compiler to decide.

    > +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?

    > +
    > + 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. 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. 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.

    > +
    > + 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.

    Andrew

    \
     
     \ /
      Last update: 2023-03-27 00:47    [W:3.523 / U:0.128 seconds]
    ©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site