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
| ||
|
Message-ID: <7ad568dc151647a7481a6ddd3c402c0b79277896.camel@microchip.com> Date: Thu, 15 Sep 2022 04:43:45 +0000 From: <Arun.Ramadoss@...rochip.com> To: <linux@...linux.org.uk> CC: <andrew@...n.ch>, <linux-kernel@...r.kernel.org>, <UNGLinuxDriver@...rochip.com>, <vivien.didelot@...il.com>, <olteanv@...il.com>, <Tristram.Ha@...rochip.com>, <f.fainelli@...il.com>, <Prasanna.VengateshanVaradharajan@...rochip.com>, <kuba@...nel.org>, <edumazet@...gle.com>, <pabeni@...hat.com>, <netdev@...r.kernel.org>, <Woojung.Huh@...rochip.com>, <davem@...emloft.net>, <hkallweit1@...il.com> Subject: Re: [Patch net-next v2 4/5] net: dsa: microchip: move interrupt handling logic from lan937x to ksz_common Hi Russel, Thanks for the comment. On Wed, 2022-09-14 at 12:27 +0100, Russell King (Oracle) wrote: > EXTERNAL EMAIL: Do not click links or open attachments unless you > know the content is safe > > Hi, > > Some suggestions for a few improvements in a future patch: > > On Wed, Sep 14, 2022 at 09:22:22AM +0530, Arun Ramadoss wrote: > > +static int ksz_irq_phy_setup(struct ksz_device *dev) > > +{ > > + struct dsa_switch *ds = dev->ds; > > + int phy, err_phy; > > + int irq; > > + int ret; > > + > > + for (phy = 0; phy < KSZ_MAX_NUM_PORTS; phy++) { > > + if (BIT(phy) & ds->phys_mii_mask) { > > + irq = irq_find_mapping(dev- > > >ports[phy].pirq.domain, > > + PORT_SRC_PHY_INT); > > + if (irq < 0) { > > + ret = irq; > > + goto out; > > + } > > + ds->slave_mii_bus->irq[phy] = irq; > > + } > > + } > > + return 0; > > +out: > > + err_phy = phy; > > + > > + for (phy = 0; phy < err_phy; phy++) > > + if (BIT(phy) & ds->phys_mii_mask) > > + irq_dispose_mapping(ds->slave_mii_bus- > > >irq[phy]); > > while (phy--) > if (BIT(phy) & ds->phys_mii_mask) > irq_dispose_mapping(ds->slave_mii_bus- > >irq[phy]); Ok. I will update. > > ? > > > +static void ksz_girq_mask(struct irq_data *d) > > +{ > > + struct ksz_device *dev = irq_data_get_irq_chip_data(d); > > + unsigned int n = d->hwirq; > > + > > + dev->girq.masked |= (1 << n); > > dev->girq.masked |= BIT(d->hwirq); > > ? > > > +} > > + > > +static void ksz_girq_unmask(struct irq_data *d) > > +{ > > + struct ksz_device *dev = irq_data_get_irq_chip_data(d); > > + unsigned int n = d->hwirq; > > + > > + dev->girq.masked &= ~(1 << n); > > dev->girq.masked &= ~BIT(d->hw_irq); Ok. I will replace them with macros. > > ? > > > +} > > + > > +static void ksz_girq_bus_lock(struct irq_data *d) > > +{ > > + struct ksz_device *dev = irq_data_get_irq_chip_data(d); > > + > > + mutex_lock(&dev->lock_irq); > > +} > > + > > +static void ksz_girq_bus_sync_unlock(struct irq_data *d) > > +{ > > + struct ksz_device *dev = irq_data_get_irq_chip_data(d); > > + int ret; > > + > > + ret = ksz_write32(dev, REG_SW_PORT_INT_MASK__4, dev- > > >girq.masked); > > + if (ret) > > + dev_err(dev->dev, "failed to change IRQ mask\n"); > > + > > + mutex_unlock(&dev->lock_irq); > > +} > > + > > +static const struct irq_chip ksz_girq_chip = { > > + .name = "ksz-global", > > + .irq_mask = ksz_girq_mask, > > + .irq_unmask = ksz_girq_unmask, > > + .irq_bus_lock = ksz_girq_bus_lock, > > + .irq_bus_sync_unlock = ksz_girq_bus_sync_unlock, > > +}; > > As the pirq code is almost identical to the girq code, how about > putting > a "reg_mask", "reg_status" and a pointer to ksz_device into ksz_irq, > and > using the ksz_irq as the chip data? > > These would then become: > > static void ksz_irq_mask(struct irq_data *d) > { > struct ksz_irq *ki = irq_data_get_irq_chip_data(d); > > ki->masked |= BIT(d->hwirq); > } > > static void ksz_irq_unmask(struct irq_data *d) > { > struct ksz_irq *ki = irq_data_get_irq_chip_data(d); > > ki->masked &= ~BIT(d->hwirq); > } > > static void ksz_irq_bus_lock(struct irq_data *d) > { > struct ksz_irq *ki = irq_data_get_irq_chip_data(d); > > mutex_lock(&ki->dev->lock_irq); > } > > static void ksz_irq_bus_sync_unlock(struct irq_data *d) > { > struct ksz_irq *ki = irq_data_get_irq_chip_data(d); > struct ksz_device *dev = ki->dev; > int ret; > > ret = ksz_write32(dev, ki->reg_masked, ki->masked); > if (ret) > dev_err(dev->dev, "failed to change IRQ mask\n"); > > mutex_unlock(&dev->lock_irq); > } > > and thus this code could be shared between both pirq and girq. > I'm pretty sure the thead_fn could be shared as well, and I'm > sure that the setup and tear down could be improved in a similar > way. Thanks for the suggestion. I will update the code and send the next version of the patch. > > -- > RMK's Patch system: https://www.armlinux.org.uk/developer/patches/ > FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!
Powered by blists - more mailing lists