From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marc Kleine-Budde Subject: Re: [PATCH 5/7] can: dev: add berr_limit infrastrucutre Date: Mon, 07 Oct 2013 17:56:46 +0200 Message-ID: <5252D9BE.9040905@pengutronix.de> References: <1381156840-24071-1-git-send-email-mkl@pengutronix.de> <1381156840-24071-6-git-send-email-mkl@pengutronix.de> <1876232.tLyIP8q8GR@ws-stein> Mime-Version: 1.0 Content-Type: multipart/signed; micalg=pgp-sha1; protocol="application/pgp-signature"; boundary="gUxsUJ55aPse3afr3CaLTQHpuTxJN2uRo" Return-path: Received: from metis.ext.pengutronix.de ([92.198.50.35]:50637 "EHLO metis.ext.pengutronix.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755821Ab3JGP44 (ORCPT ); Mon, 7 Oct 2013 11:56:56 -0400 In-Reply-To: <1876232.tLyIP8q8GR@ws-stein> Sender: linux-can-owner@vger.kernel.org List-ID: To: Alexander Stein Cc: linux-can@vger.kernel.org, kernel@pengutronix.de This is an OpenPGP/MIME signed message (RFC 4880 and 3156) --gUxsUJ55aPse3afr3CaLTQHpuTxJN2uRo Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable On 10/07/2013 05:39 PM, Alexander Stein wrote: > Hello Marc, >=20 > On Monday 07 October 2013 16:40:38, Marc Kleine-Budde wrote: >> diff --git a/drivers/net/can/dev.c b/drivers/net/can/dev.c >> index bda1888..969d3cd 100644 >> --- a/drivers/net/can/dev.c >> +++ b/drivers/net/can/dev.c >> @@ -486,6 +486,29 @@ void can_bus_off(struct net_device *dev) >> } >> EXPORT_SYMBOL_GPL(can_bus_off); >> =20 >> +static void can_berr_restart(unsigned long data) >> +{ >> + struct net_device *dev =3D (struct net_device *)data; >> + struct can_priv *priv =3D netdev_priv(dev); >> + >> + netdev_dbg(dev, "berr-restart\n"); >> + priv->do_berr_restart(dev); > ^^^^^^^^^^^^^^^^^^^^^^^^ > see below! >=20 >> +} >> + >> +void can_berr_limit(struct net_device *dev) >> +{ >> + struct can_priv *priv =3D netdev_priv(dev); >> + >> + if (priv->berr_limit_delay) { >> + netdev_dbg(dev, "berr-limit\n"); >> + mod_timer(&priv->berr_limit_timer, >> + jiffies + priv->berr_limit_delay); >> + } else { >> + priv->do_berr_restart(dev); > ^^^^^^^^^^^^^^^^^^^^^^^^^^^ > Here you are silently requiring that do_berr_restart is non-NULL. I > know that the driver has to properly set this when using > can_berr_limit, but it might seem confusing. If the driver wants to make use of this feature, it has to setup this callback. If I add NULL pointer checks, the kernel wouldn't blow up, but the driver doesn't work any more. If this callback isn't setup, the userspace cannot activate the feature via netlink. Maybe we can define a control mode that the driver supports this feature and during can_open() it's checked if the callback is !=3D NULL. Than "ifconfig up" will fail, the driver doesn't work, same as a NULL pointer deref later, but coated in sugar :) Maybe we find a better way. thanks for the review, Marc --=20 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 | --gUxsUJ55aPse3afr3CaLTQHpuTxJN2uRo Content-Type: application/pgp-signature; name="signature.asc" Content-Description: OpenPGP digital signature Content-Disposition: attachment; filename="signature.asc" -----BEGIN PGP SIGNATURE----- Version: GnuPG v1.4.14 (GNU/Linux) Comment: Using GnuPG with Icedove - http://www.enigmail.net/ iEYEARECAAYFAlJS2b4ACgkQjTAFq1RaXHN/DgCeIiythmRcMuQ7v8kHjtVXOaKa mPEAn2+XQeCTFaspQBQwYaqkECgK2WpC =rmNP -----END PGP SIGNATURE----- --gUxsUJ55aPse3afr3CaLTQHpuTxJN2uRo--