[<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 netdev" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Powered by blists - more mailing lists