On 2/21/19 2:03 PM, tristram...@microchip.com wrote:
> From: Tristram Ha <tristram...@microchip.com>
>
> Get port link status to know whether to read MIB counters when the link
> is going down. Add port_cleanup function to read MIB counters the last
> time as when the port is disabled the PHY is also powered down.
>
> Signed-off-by: Tristram Ha <tristram...@microchip.com>
> ---
Some comments here and there again, not against a merge, but would be
nice to address in the future.
[snip]
> +void ksz_port_cleanup(struct ksz_device *dev, int port)
> +{
> + /* Read all MIB counters when the link is going down. */
> + if (dev->live_ports & (1 << port)) {
> + struct ksz_port *p = &dev->ports[port];
> +
> + p->read = true;
> + schedule_work(&dev->mib_read);
> + }
> +
> + /* Common code for port cleanup. */
> + dev->on_ports &= ~(1 << port);
It seems to be that dev->on_ports is a shorthand for a port is enabled
whether its link is up or down, which is equivalent to using:
!dsa_port_is_unused(ds, port)) unless I am mistaken about the intention
of the code.
In general you seem to be very prone to adding a lot of these
bookkeeping variables which just creates more opportunities for bugs to
creep in.
> + dev->live_ports &= ~(1 << port);
> +}
> +EXPORT_SYMBOL_GPL(ksz_port_cleanup);
> +
> void ksz_update_port_member(struct ksz_device *dev, int port)
> {
> struct ksz_port *p;
> @@ -156,6 +172,26 @@ int ksz_phy_write16(struct dsa_switch *ds, int addr, int
> reg, u16 val)
> }
> EXPORT_SYMBOL_GPL(ksz_phy_write16);
>
> +void ksz_adjust_link(struct dsa_switch *ds, int port,
> + struct phy_device *phydev)
> +{
> + struct ksz_device *dev = ds->priv;
> + struct ksz_port *p = &dev->ports[port];
> +
> + if (!phydev->link) {
> + /* Read all MIB counters when the link is going down. */
> + if (dev->live_ports & (1 << port)) {
> + p->read = true;
> + schedule_work(&dev->mib_read);
> + }
> + dev->live_ports &= ~(1 << port);
> + } else {
> + /* Remember which port is connected and active. */
> + dev->live_ports |= (1 << port) & dev->on_ports;
> + }
How about something simpler than this:
if (phydev->link != p->old_link) {
p->read = !!phydev->link;
schedule_work(&dev->mib_read);
}
This is also super racy because you schedule the workqueue without any
locking against the state machine which runs with the PHY device's mutex
held, so there is the possibility of the following happening:
Thread 0 Thread 1
ksz_adjust_link()
phydev->link = 0
schedule_work()
ksz_adjust_link()
p->read = true
mib_read_workqueue()
and so we don't read counters anymore. It is probably fine since this is
clearly entirely opportunistic and in order to minimize register bandwidth.
--
Florian
