netdev - Re: [PATCH v7 3/3] can: Allwinner A10/A20 CAN Controller support

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]

Message-ID: <55F82F49.8020901@pengutronix.de>
Date:	Tue, 15 Sep 2015 16:46:33 +0200
From:	Marc Kleine-Budde <mkl@...gutronix.de>
To:	Maxime Ripard <maxime.ripard@...e-electrons.com>,
	Gerhard Bertelsmann <info@...hard-bertelsmann.de>
Cc:	wg@...ndegger.com, linux-can@...r.kernel.org,
	netdev@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v7 3/3] can: Allwinner A10/A20 CAN Controller support -
 Kernel module

On 09/15/2015 04:28 PM, Maxime Ripard wrote:
>> +static int sunxican_get_berr_counter(const struct net_device *dev,
>> +				     struct can_berr_counter *bec)
>> +{
>> +	struct sunxican_priv *priv = netdev_priv(dev);
>> +	u32 errors;
>> +	int err;
>> +
>> +	err = clk_prepare_enable(priv->clk);
>> +	if (err) {
>> +		netdev_err(dev, "could not enable clock\n");
>> +		return err;
>> +	}
> 
> Can this function be called if the device is closed? If not, then this
> clock calls are useless.

Yes, that's why there are there.

> 
>> +	errors = readl(priv->base + SUNXI_REG_ERRC_ADDR);
>> +
>> +	bec->txerr = errors & 0xFF;
>> +	bec->rxerr = (errors >> 16) & 0xFF;
>> +
>> +	clk_disable_unprepare(priv->clk);
>> +
>> +	return 0;
>> +}

[...]

>> +static int sunxican_open(struct net_device *dev)
>> +{
>> +	struct sunxican_priv *priv = netdev_priv(dev);
>> +	int err;
>> +
>> +	/* common open */
>> +	err = open_candev(dev);
>> +	if (err)
>> +		return err;
>> +
>> +	/* register interrupt handler */
>> +	err = request_irq(dev->irq, sunxi_can_interrupt, IRQF_SHARED,
> 
> We don't have any shared interrupt as far as I know, do you really
> need this flag?

Not needed, but the IRQ handler properly returns HANDLED or NONE. So
it's better to remove this flag then?

> 
>> +			  dev->name, dev);
>> +	if (err) {
>> +		netdev_err(dev, "request_irq err: %d\n", err);
>> +		goto exit_irq;
>> +	}
>> +
>> +	/* turn on clocking for CAN peripheral block */
>> +	err = clk_prepare_enable(priv->clk);
>> +	if (err) {
>> +		netdev_err(dev, "could not enable CAN peripheral clock\n");
>> +		goto exit_clock;
>> +	}
>> +
>> +	err = sunxi_can_start(dev);
>> +	if (err) {
>> +		netdev_err(dev, "could not start CAN peripheral\n");
>> +		goto exit_can_start;
>> +	}
>> +
>> +	can_led_event(dev, CAN_LED_EVENT_OPEN);
>> +	netif_start_queue(dev);
>> +
>> +	return 0;
>> +
>> +exit_can_start:
>> +	clk_disable_unprepare(priv->clk);
>> +exit_clock:
>> +	free_irq(dev->irq, dev);
>> +exit_irq:
>> +	close_candev(dev);
>> +	return err;
>> +}

Marc

-- 
Pengutronix e.K.                  | Marc Kleine-Budde           |
Industrial Linux Solutions        | Phone: +49-231-2826-924     |
Vertretung West/Dortmund          | Fax:   +49-5121-206917-5555 |
Amtsgericht Hildesheim, HRA 2686  | http://www.pengutronix.de   |


Download attachment "signature.asc" of type "application/pgp-signature" (456 bytes)