netdev - RE: [PATCH net-next] lan78xx: Use irq_domain for phy interrupt from USB Int. EP

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 28 Oct 2016 20:50:49 +0000
From:   <Woojung.Huh@...rochip.com>
To:     <f.fainelli@...il.com>, <davem@...emloft.net>,
        <netdev@...r.kernel.org>, <andrew@...n.ch>
CC:     <UNGLinuxDriver@...rochip.com>
Subject: RE: [PATCH net-next] lan78xx: Use irq_domain for phy interrupt from
 USB Int. EP

> > +static void lan78xx_irq_mask(struct irq_data *irqd)
> > +{
> > +	struct irq_domain_data *data = irq_data_get_irq_chip_data(irqd);
> > +	struct lan78xx_net *dev =
> > +			container_of(data, struct lan78xx_net, domain_data);
> > +	u32 buf;
> > +
> > +	lan78xx_read_reg(dev, INT_EP_CTL, &buf);
> 
> lan78xx_read_reg() uses kmalloc() with GFP_KERNEL, while
> irq_mask/irq_unmask can be called in atomic context AFAIR, you may need
> to pass down a specific gfp_t to lan78xx_read_reg.
> 
> What about usb_submit_urb(), can that work in atomic context? Do you
> need to have lan78xx_read_reg() acquire a raw spinlock or something to
> serialize them?
> 
> > +	buf &= ~INT_EP_PHY_INT_EN_;
> 
> Even though you may have only one interrupt line to deal with at the
> moment, better make this bit derived from irqd->hwirq instead of hard
> coding it here.
> 
> > +	lan78xx_write_reg(dev, INT_EP_CTL, buf);
> > +}
> > +
> > +static void lan78xx_irq_unmask(struct irq_data *irqd)
> > +{
> > +	struct irq_domain_data *data = irq_data_get_irq_chip_data(irqd);
> > +	struct lan78xx_net *dev =
> > +			container_of(data, struct lan78xx_net, domain_data);
> > +	u32 buf;
> > +
> > +	lan78xx_read_reg(dev, INT_EP_CTL, &buf);
> > +	buf |= INT_EP_PHY_INT_EN_;
> 
> Same here, this should come from irqd->hwirq.
> 
> > +	lan78xx_write_reg(dev, INT_EP_CTL, buf);
> > +}
> > +
> > +static struct irq_chip lan78xx_irqchip = {
> > +	.name		= "lan78xx-phyirq",
> > +	.irq_mask	= lan78xx_irq_mask,
> > +	.irq_unmask	= lan78xx_irq_unmask,
> > +};
> > +
> > +static int lan78xx_setup_irq_domain(struct lan78xx_net *dev)
> > +{
> > +	struct device_node *of_node;
> > +	struct irq_domain *irqdomain;
> > +	unsigned int irq_base = 0;
> > +	int ret = 0;
> > +
> > +	of_node = dev->udev->dev.parent->of_node;
> > +
> > +	dev->domain_data.irqchip = &lan78xx_irqchip;
> > +	dev->domain_data.irq_handler = handle_simple_irq;
> > +
> > +	irqdomain = irq_domain_add_simple(of_node, 1, 0,
> &chip_domain_ops,
> > +					  &dev->domain_data);
> 
> Is there really just one interrupt associated with this peripheral here?
> 
> >
> > +	if (lan78xx_setup_irq_domain(dev) < 0) {
> > +		netdev_warn(dev->net, "lan78xx_setup_irq_domain()
> failed");
> > +		return -EIO;
> > +	}
> 
> Any reason not to propagate the error code from
> lan78xx_setup_irq_domain() here?

Thanks for prompt review.
I'll look into it and submit update.

Thanks.
- Woojung