netdev - Re: [PATCH v5 2/2] Ethernet driver for the WIZnet W5100 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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <4F797402.1050109@permonline.ru>
Date:	Mon, 02 Apr 2012 15:40:18 +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 v5 2/2] Ethernet driver for the WIZnet W5100 chip

01.04.2012 3:23, Mark Brown wrote:
> On Fri, Mar 30, 2012 at 01:00:06PM +0600, Mike Sinkovsky wrote:
>
>> +config WIZNET_W5100
>> +	tristate "WIZnet W5100 Ethernet support"
>> +	depends on ARM || BLACKFIN
>
> What are the architecture dependencies here?

Original driver from chip manufacturer was written for ARM, and now we 
use it on Blackfin.
Completely untested on other arch's, but should work. Maybe.


>> +static irqreturn_t w5100_interrupt(int irq, void *ndev_instance)
>> +{
>> +	struct net_device *ndev = ndev_instance;
>> +	struct w5100_priv *priv = netdev_priv(ndev);
>> +
>> +	int ir = w5100_read(priv, W5100_S0_IR);
>> +	w5100_write(priv, W5100_S0_IR, ir);
>> +
>> +	if (ir&  S0_IR_RECV) {
>> +		if (napi_schedule_prep(&priv->napi)) {
>> +			w5100_write(priv, W5100_IMR, 0);
>> +			mmiowb();
>> +			__napi_schedule(&priv->napi);
>> +		}
>> +	}
>> +
>> +	if (ir&  S0_IR_SENDOK) {
>> +		if (unlikely(netif_queue_stopped(ndev)))
>> +			netif_wake_queue(ndev);
>> +	}
>> +
>> +	return IRQ_HANDLED;
>
> This unconditionally acknowledges the interrupt even if one wasn't
> reported by the device.

Hm? Don't get you.
W5100_S0_IR register is R/W1C - writing back clears it.
Or what do you mean?


>> +	irq = platform_get_irq(pdev, 0);
>> +	if (irq<  0)
>> +		return irq;
>> +	ret = devm_request_irq(&pdev->dev, irq, w5100_interrupt,
>> +			       IRQ_TYPE_LEVEL_LOW, name, ndev);
>> +	if (ret<  0)
>> +		return ret;
>> +	priv->irq = irq;
>> +
>> +	link = platform_get_resource(pdev, IORESOURCE_IO, 0);
>> +	if (!link) {
>> +		priv->link_gpio = -1;
>
> This is rather an abuse of the resource API and will run into trouble on
> device tree based systems.  You should use platform data for non-DT
> systems.

Ok, will move it to struct wiznet_platform_data.
But here is downside - this gpio is optional, and if board doesn't have 
it - it must be initialized as negative value, not just omitted.


>> +		if (request_irq(priv->link_irq, w5100_detect_link,
>> +				IRQF_TRIGGER_RISING | IRQF_TRIGGER_FALLING,
>> +				link_name, priv->ndev)<  0)
>
> Suggest using request_any_context_irq() to increase the range of
> supported interrupt controllers.

Could it be anything but hard irq?
But there is another bug - it should be devm_request_irq...
:)


>> +err_register:
>> +	free_netdev(ndev);
>> +	platform_set_drvdata(pdev, NULL);
>> +	dev_info(&pdev->dev, "probe failed (%d)\n", err);
>
> This will be done for you by the driver core.

You mean platform_set_drvdata() and dev_info()? Maybe.
I'm sure platform_driver will not do free_netdev() for me.


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