netdev - Re: [ath5k-devel] [PATCH] [ath5k][leds] Ability to disable leds support. If leds support enabled do not force mac802.11 leds layer selection.

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: <AANLkTinYfKlQ-MzkGV9o2qXUTeArYscfqoc8WS7SRjgz@mail.gmail.com>
Date:	Thu, 13 May 2010 10:36:56 +0300
From:	Dmytro Milinevskyy <milinevskyy@...il.com>
To:	Pavel Roskin <proski@....org>
Cc:	ath5k-devel@...ts.ath5k.org, Kalle Valo <kalle.valo@....fi>,
	linux-wireless@...r.kernel.org,
	GeunSik Lim <geunsik.lim@...sung.com>,
	Jiri Slaby <jirislaby@...il.com>,
	Greg Kroah-Hartman <gregkh@...e.de>,
	"John W. Linville" <linville@...driver.com>,
	Keng-Yu Lin <keng-yu.lin@...onical.com>,
	netdev@...r.kernel.org, Jiri Kosina <jkosina@...e.cz>,
	Johannes Berg <johannes@...solutions.net>,
	Shahar Or <shahar@...har-or.co.il>,
	linux-kernel@...r.kernel.org, Luca Verdesca <magooz@...ug.it>
Subject: Re: [ath5k-devel] [PATCH] [ath5k][leds] Ability to disable leds 
	support. If leds support enabled do not force mac802.11 leds layer selection.

Hello, Pavel.

I will rework the patch considering your suggestions.

> I'm not sure this complexity is needed.  Are you going to support LEDs
> if CONFIG_LEDS_CLASS is disabled?
If there's any other place in driver that might want use LEDs w/o
exporting the interface to the userspace.

-- Dima Milinevskyy


On Wed, May 12, 2010 at 6:48 PM, Pavel Roskin <proski@....org> wrote:
> On Wed, 2010-04-07 at 21:58 +0300, Dmytro Milinevskyy wrote:
>
>> Here is the patch to disable ath5k leds support on build stage.
>> However if the leds support was enabled do not force selection of 802.11 leds layer.
>
> The idea is good, but the implementation could be improved.
>
> There are too many preprocessor conditionals in your patch.
>
>> +#ifdef CONFIG_ATH5K_LEDS
>>  /*
>>   * These match net80211 definitions (not used in
>>   * mac80211).
>> @@ -939,11 +940,7 @@ enum ath5k_power_mode {
>>  #define AR5K_LED_AUTH        2 /*IEEE80211_S_AUTH*/
>>  #define AR5K_LED_ASSOC       3 /*IEEE80211_S_ASSOC*/
>>  #define AR5K_LED_RUN 4 /*IEEE80211_S_RUN*/
>
> It should be OK to leave the constants defined even if they are not
> used.
>
>> +#ifdef CONFIG_ATH5K_LEDS
>>  /* LED functions */
>>  extern int ath5k_init_leds(struct ath5k_softc *sc);
>>  extern void ath5k_led_enable(struct ath5k_softc *sc);
>>  extern void ath5k_led_off(struct ath5k_softc *sc);
>>  extern void ath5k_unregister_leds(struct ath5k_softc *sc);
>> +#endif
>
> You could add inline functions for the case when CONFIG_ATH5K_LEDS is
> not defined.  That would avoid may conditionals in the code.
>
>>  /* GPIO Functions */
>> +#ifdef CONFIG_ATH5K_LEDS
>>  extern void ath5k_hw_set_ledstate(struct ath5k_hw *ah, unsigned int state);
>> +#endif
>
> The same comment applies.
>
> Also, there is nothing wrong with having an external declaration that is
> not used in some particular configuration.
>
>> +#ifdef CONFIG_ATH5K_LEDS
>>       /* turn on HW LEDs */
>>       ath5k_hw_set_ledstate(ah, AR5K_LED_INIT);
>> +#endif
>
> This is avoidable by having an inline ath5k_hw_set_ledstate() that does
> nothing.
>
>> +#ifdef CONFIG_ATH5K_LEDS
>>       struct ieee80211_hw *hw = pci_get_drvdata(to_pci_dev(dev));
>>       struct ath5k_softc *sc = hw->priv;
>>
>>       ath5k_led_off(sc);
>> +#endif
>
> Even this is avoidable if ath5k_led_off() does nothing.  gcc should be
> smart enough to optimize out unneeded function calls.
>
>> +#ifdef CONFIG_ATH5K_LEDS
>>  /*
>>   * State for LED triggers
>>   */
>>  struct ath5k_led
>>  {
>> +#ifdef CONFIG_LEDS_CLASS
>
> I'm not sure this complexity is needed.  Are you going to support LEDs
> if CONFIG_LEDS_CLASS is disabled?
>
>> +#ifdef CONFIG_ATH5K_LEDS
>>       unsigned int            led_pin,        /* GPIO pin for driving LED */
>>                               led_on;         /* pin setting for LED on */
>> +#endif
>>
>>       struct tasklet_struct   restq;          /* reset tasklet */
>>
>> @@ -164,7 +172,9 @@ struct ath5k_softc {
>>       spinlock_t              rxbuflock;
>>       u32                     *rxlink;        /* link ptr in last RX desc */
>>       struct tasklet_struct   rxtq;           /* rx intr tasklet */
>> +#ifdef CONFIG_ATH5K_LEDS
>>       struct ath5k_led        rx_led;         /* rx led */
>> +#endif
>
> You may want to group those fields together to make the code more
> readable.
>
>> --- a/drivers/net/wireless/ath/ath5k/led.c
>> +++ b/drivers/net/wireless/ath/ath5k/led.c
>
> I wonder if you could omit led.c completely in the Makefile.  If there
> are some parts of led.c that are needed without CONFIG_ATH5K_LEDS, maybe
> they belong elsewhere?
>
> --
> Regards,
> Pavel Roskin
>
--
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