linux-kernel - Re: [PATCH can-next v5 1/2] can: add tx/rx LED trigger 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <1920193464.85058.1343822773242.JavaMail.open-xchange@webmail.strato.de>
Date:	Wed, 1 Aug 2012 14:06:13 +0200 (CEST)
From:	Oliver Hartkopp <socketcan@...tkopp.net>
To:	linux-can@...r.kernel.org,
	Fabio Baltieri <fabio.baltieri@...il.com>
Cc:	linux-kernel@...r.kernel.org,
	Wolfgang Grandegger <wg@...ndegger.com>,
	Marc Kleine-Budde <mkl@...gutronix.de>
Subject: Re: [PATCH can-next v5 1/2] can: add tx/rx LED trigger support

Sorry for this potentially mangled mail from my webmail access ...

Fabio Baltieri <fabio.baltieri@...il.com> hat am 1. August 2012 um 13:49
geschrieben:

> +void devm_can_led_init(struct net_device *netdev)
> +{
> +  struct can_priv *priv = netdev_priv(netdev);
> +  void *res;
> +
> +  res = devres_alloc(can_led_release, 0, GFP_KERNEL);
> +  if (!res)
> +          goto err_out;
> +
> +  priv->tx_led_trig_name = kasprintf(GFP_KERNEL, "%s-tx", netdev->name);

IMO putting a string with 8 or 9 bytes into a separate kmalloc memory sniplet is
pretty oversized.
Ok - these functions provide to hide the complexitiy for allocating and storing
strings, which is
definitely fine for path names and these kind of strings that are not known in
length and probably
more than 100 bytes long.

But in this case i would suggest to allocate a fixed space in can_priv, as we
know the string length
very good (IFNAMSZ + strlen("-tx")) and there's no reason to get all the
overhead from three kmallocs
instead of one for that small memory allocations.

So i would suggest:

> @@ -52,6 +53,13 @@ struct can_priv {
> 
>    unsigned int echo_skb_max;
>    struct sk_buff **echo_skb;
> +
> +#ifdef CONFIG_CAN_LEDS
> +  struct led_trigger *tx_led_trig;
> +  char *tx_led_trig_name;

char tx_led_trig_name[IFNAMSZ+4];

> +  struct led_trigger *rx_led_trig;
> +  char *rx_led_trig_name;

char rx_led_trig_name[IFNAMSZ+4];

> +#endif
>  };
> 

Just my two cents as i was in CC here :-)

Thanks for the cool LED support & best regards
Oliver
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/