03.04.2012 19:36, Mark Brown написал:

On Tue, Apr 03, 2012 at 04:58:45PM +0600, Mike Sinkovsky wrote:

quoted

+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...

quoted

+	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