linux-kernel - Re: [PATCH v6 1/2] Ethernet driver for the WIZnet W5300 chip

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 for Android: free password hash cracker in your pocket

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

Date:	Wed, 04 Apr 2012 12:18:12 +0600
From:	Mike Sinkovsky <msink@...monline.ru>
To:	Mark Brown <broonie@...nsource.wolfsonmicro.com>
CC:	netdev@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v6 1/2] Ethernet driver for the WIZnet W5300 chip

03.04.2012 19:36, Mark Brown написал:
> On Tue, Apr 03, 2012 at 04:58:45PM +0600, Mike Sinkovsky wrote:
>
>> +static irqreturn_t w5300_detect_link(int irq, void *ndev_instance)
>> +{
>> +	struct net_device *ndev = ndev_instance;
>> +	struct w5300_priv *priv = netdev_priv(ndev);
>> +
>> +	if (netif_running(ndev)) {
>> +		if (gpio_get_value(priv->link_gpio) != 0) {
>> +			netif_info(priv, link, ndev, "link is up\n");
>> +			netif_carrier_on(ndev);
>> +		} else {
>> +			netif_info(priv, link, ndev, "link is down\n");
>> +			netif_carrier_off(ndev);
>> +		}
>> +	}
>> +
>> +	return IRQ_HANDLED;
>> +}
>
> This relies on ndev being valid and on the gpio being requested but...
>
>> +	priv->link_gpio = data->link_gpio;
>> +	if (gpio_is_valid(priv->link_gpio)) {
>> +		char *link_name = devm_kzalloc(&pdev->dev, 16, GFP_KERNEL);
>> +		if (!link_name)
>> +			return -ENOMEM;
>> +		snprintf(link_name, 16, "%s-link", name);
>> +		priv->link_irq = gpio_to_irq(priv->link_gpio);
>> +		if (devm_request_threaded_irq(&pdev->dev,
>> +				priv->link_irq, NULL, w5300_detect_link,
>> +				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
>> +				link_name, priv->ndev)<  0)
>> +			priv->link_gpio = -EINVAL;
>> +	}
>
> ...you use devm_request_threaded_irq() here and rely on it for cleanup.
> Are you sure there's no possibility of the interrupt firing after you
> start to tear down the device?
>
> By using a specifically threaded IRQ you're also adding a performance
> overhead for no good reason if you can call netdev_carrier_*() from IRQ
> context and the GPIO is capable of generating a hard IRQ.  If you use
> request_any_context_irq() instead then the driver will get a hard IRQ if
> that's supported.

There isn't devm* variant of request_any_context_irq(), and using plain 
version looks inconsistent with other resources handling.
Anyway, this is not performance critical procedure, and latency around 
100 millisecond is acceptable. Some our boards even don't have this gpio 
at all, and nothing bad happens, just userspace doesn't know is carrier 
on or off.

Some gpio cannot generate irq at all, and if goal is to support any - I 
can easily convert this to background polling from delayed_work 
procedure. If it is required for generalized mainstream driver.


-- 
Mike

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/