| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Thu, 1 Sep 2022 09:02:51 +0000
From: <Arun.Ramadoss@...rochip.com>
To: <andrew@...n.ch>
CC: <olteanv@...il.com>, <linux-kernel@...r.kernel.org>,
<UNGLinuxDriver@...rochip.com>, <vivien.didelot@...il.com>,
<linux@...linux.org.uk>, <Tristram.Ha@...rochip.com>,
<f.fainelli@...il.com>, <kuba@...nel.org>, <edumazet@...gle.com>,
<pabeni@...hat.com>, <netdev@...r.kernel.org>,
<Woojung.Huh@...rochip.com>, <davem@...emloft.net>
Subject: Re: [RFC Patch net-next v3 3/3] net: dsa: microchip: lan937x: add
interrupt support for port phy link
On Tue, 2022-08-30 at 15:25 +0200, Andrew Lunn wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
>
> > @@ -85,6 +94,7 @@ struct ksz_port {
> > u32 rgmii_tx_val;
> > u32 rgmii_rx_val;
> > struct ksz_device *ksz_dev;
> > + struct ksz_irq irq;
>
> Here irq is of type ksz_irq.
>
> > u8 num;
> > };
> >
> > @@ -103,6 +113,7 @@ struct ksz_device {
> > struct regmap *regmap[3];
> >
> > void *priv;
> > + int irq;
>
> Here it is of type int.
>
> >
> > struct gpio_desc *reset_gpio; /* Optional reset GPIO */
> >
> > @@ -124,6 +135,8 @@ struct ksz_device {
> > u16 mirror_tx;
> > u32 features; /* chip specific features */
> > u16 port_mask;
> > + struct mutex lock_irq; /* IRQ Access */
> > + struct ksz_irq girq;
>
> And here you have the type ksz_irq called qirq. This is going to be
> confusing.
>
> I suggest you make the first one called pirq, for port, and then we
> have girq for global, and irq is just a number.
Ok. I will update variable names as per the context it is used.
>
> > static int lan937x_cfg(struct ksz_device *dev, u32 addr, u8 bits,
> > bool set)
> > {
> > return regmap_update_bits(dev->regmap[0], addr, bits, set ?
> > bits : 0);
> > @@ -171,6 +175,7 @@ static int lan937x_mdio_register(struct
> > ksz_device *dev)
> > struct device_node *mdio_np;
> > struct mii_bus *bus;
> > int ret;
> > + int p;
> >
> > mdio_np = of_get_child_by_name(dev->dev->of_node, "mdio");
> > if (!mdio_np) {
> > @@ -194,6 +199,16 @@ static int lan937x_mdio_register(struct
> > ksz_device *dev)
> >
> > ds->slave_mii_bus = bus;
> >
> > + for (p = 0; p < KSZ_MAX_NUM_PORTS; p++) {
> > + if (BIT(p) & ds->phys_mii_mask) {
> > + unsigned int irq;
> > +
> > + irq = irq_find_mapping(dev-
> > >ports[p].irq.domain,
> > + PORT_SRC_PHY_INT);
>
> This could return an error code. You really should check for it, the
> irq subsystem is not going to be happy with a negative irq number.
Ok. I will check the return value for irq_find_mapping.
>
> > + ds->slave_mii_bus->irq[p] = irq;
> > + }
> > + }
> > +
> > ret = devm_of_mdiobus_register(ds->dev, bus, mdio_np);
> > if (ret) {
> > dev_err(ds->dev, "unable to register MDIO bus %s\n",
>
> I don't see anywhere you destroy the mappings you created above when
> the MDIO bus is unregistered. The equivalent of
> mv88e6xxx_g2_irq_mdio_free().
I missed to destroy it. I will add.
>
>
> > +static irqreturn_t lan937x_girq_thread_fn(int irq, void *dev_id)
> > +{
> > + struct ksz_device *dev = dev_id;
> > + unsigned int nhandled = 0;
> > + unsigned int sub_irq;
> > + unsigned int n;
> > + u32 data;
> > + int ret;
> > +
> > + ret = ksz_read32(dev, REG_SW_INT_STATUS__4, &data);
> > + if (ret)
> > + goto out;
> > +
> > + if (data & POR_READY_INT) {
> > + ret = ksz_write32(dev, REG_SW_INT_STATUS__4,
> > POR_READY_INT);
> > + if (ret)
> > + goto out;
> > + }
>
> What do these two read/writes do? It seems like you are discarding an
> interrupt?
This interrupt in Power on reset interrupt. It is enabled by default in
the chip. I am not performing any operation based on POR. So I just
cleared the interrupt. Do I need to disable the POR interrupt in the
setup function, since no operation is performed based on it?
>
>
> > +
> > + /* Read global interrupt status register */
> > + ret = ksz_read32(dev, REG_SW_PORT_INT_STATUS__4, &data);
> > + if (ret)
> > + goto out;
> > +
> > + for (n = 0; n < dev->girq.nirqs; ++n) {
> > + if (data & (1 << n)) {
> > + sub_irq = irq_find_mapping(dev->girq.domain,
> > n);
> > + handle_nested_irq(sub_irq);
> > + ++nhandled;
> > + }
> > + }
> > +out:
> > + return (nhandled > 0 ? IRQ_HANDLED : IRQ_NONE);
> > +}
> > +
> > +static irqreturn_t lan937x_pirq_thread_fn(int irq, void *dev_id)
> > +{
> > + struct ksz_port *port = dev_id;
> > + unsigned int nhandled = 0;
> > + struct ksz_device *dev;
> > + unsigned int sub_irq;
> > + unsigned int n;
> > + u8 data;
> > +
> > + dev = port->ksz_dev;
> > +
> > + /* Read global interrupt status register */
> > + ksz_pread8(dev, port->num, REG_PORT_INT_STATUS, &data);
>
> I think global here should be port?
Yes. Copy paste problem. I will update it.
>
> > +
> > + for (n = 0; n < port->irq.nirqs; ++n) {
> > + if (data & (1 << n)) {
> > + sub_irq = irq_find_mapping(port->irq.domain,
> > n);
> > + handle_nested_irq(sub_irq);
> > + ++nhandled;
> > + }
> > + }
> > +
> > + return (nhandled > 0 ? IRQ_HANDLED : IRQ_NONE);
> > +}
> > +
> >
> > void lan937x_switch_exit(struct ksz_device *dev)
> > {
> > + struct dsa_port *dp;
> > +
> > lan937x_reset_switch(dev);
> > +
> > + if (dev->irq > 0) {
> > + dsa_switch_for_each_user_port(dp, dev->ds) {
> > + lan937x_pirq_free(&dev->ports[dp->index], dp-
> > >index);
> > + }
> > +
> > + lan937x_girq_free(dev);
>
> This is where your problem with exit vs reset is coming from. You
> setup all the interrupt code in setup(). But currently there is no
> function which is the opposite of setup(). Generally, it is called
> tairdown. Add such a function.
Yes I was looking for opposite function for setup(). Thanks for
suggestion, I will update the code accordingly and remove the patch 1/3
from the series which replaces the exit with reset.
>
> Andrew
Powered by blists - more mailing lists