Re: [PATCH v6 02/16] leds: triggers: let struct led_trigger::activate() return an error code

Jacek Anaszewski <jacek.anaszewski@xxxxxxxxx> · Mon, 2 Jul 2018 21:22:56 +0200

Hi Uwe,

On 07/02/2018 09:26 AM, Uwe Kleine-König wrote:
On Fri, Jun 29, 2018 at 10:30:37PM +0200, Uwe Kleine-König wrote:
On Fri, Jun 29, 2018 at 10:13:34PM +0200, Jacek Anaszewski wrote:
Uwe,

Thanks for resending the patch set.

I tested it with x86_64_defconfig and got following warnings:

[...]
   CC      net/rfkill/core.o
   CC      net/netfilter/nf_conntrack_seqadj.o
   CC      net/netfilter/nf_conntrack_netlink.o
net/rfkill/core.c: In function ‘rfkill_led_trigger_register’:
net/rfkill/core.c:171:31: warning: assignment from incompatible pointer type
   rfkill->led_trigger.activate = rfkill_led_trigger_activate;
                                ^
   CC      net/ipv6/sysctl_net_ipv6.o
   CC      net/ipv4/tcp_recovery.o
[...]
   CC      net/mac80211/led.o
   CC      drivers/hid/hid-topseed.o
   CC      drivers/gpu/drm/i915/i915_gem_execbuffer.o
   CC      net/ipv4/netlink.o
   AR      drivers/hid/built-in.a
   CC      net/mac80211/pm.o
net/mac80211/led.c: In function ‘ieee80211_led_init’:
net/mac80211/led.c:148:25: warning: assignment from incompatible pointer
type
   local->rx_led.activate = ieee80211_rx_led_activate;
                          ^
net/mac80211/led.c:156:25: warning: assignment from incompatible pointer
type
   local->tx_led.activate = ieee80211_tx_led_activate;
                          ^
net/mac80211/led.c:164:28: warning: assignment from incompatible pointer
type
   local->assoc_led.activate = ieee80211_assoc_led_activate;
                             ^
net/mac80211/led.c:172:28: warning: assignment from incompatible pointer
type
   local->radio_led.activate = ieee80211_radio_led_activate;
                             ^
net/mac80211/led.c:181:27: warning: assignment from incompatible pointer
type
    local->tpt_led.activate = ieee80211_tpt_led_activate;


It seems that will need net maintainer acks anyway :-)

*sigh*, this wouldn't be a stopper if the conversion wouldn't have been
done in a single step as was requested in (IIRC) v2 :-|

I found another one, the fixup is:

diff --git a/net/bluetooth/leds.c b/net/bluetooth/leds.c
index cb670b5594eb..6d59a5023231 100644
--- a/net/bluetooth/leds.c
+++ b/net/bluetooth/leds.c
@@ -43,7 +43,7 @@ void hci_leds_update_powered(struct hci_dev *hdev, bool enabled)
  	led_trigger_event(bt_power_led_trigger, enabled ? LED_FULL : LED_OFF);
  }
  
-static void power_activate(struct led_classdev *led_cdev)
+static int power_activate(struct led_classdev *led_cdev)
  {
  	struct hci_basic_led_trigger *htrig;
  	bool powered;
@@ -52,10 +52,12 @@ static void power_activate(struct led_classdev *led_cdev)
  	powered = test_bit(HCI_UP, &htrig->hdev->flags);
  
  	led_trigger_event(led_cdev->trigger, powered ? LED_FULL : LED_OFF);
+
+	return 0;
  }
  
  static struct led_trigger *led_allocate_basic(struct hci_dev *hdev,
-			void (*activate)(struct led_classdev *led_cdev),
+			int (*activate)(struct led_classdev *led_cdev),
  			const char *name)
  {
  	struct hci_basic_led_trigger *htrig;
diff --git a/net/mac80211/led.c b/net/mac80211/led.c
index ba0b507ea691..469a531e8d24 100644
--- a/net/mac80211/led.c
+++ b/net/mac80211/led.c
@@ -52,13 +52,15 @@ void ieee80211_free_led_names(struct ieee80211_local *local)
  	kfree(local->radio_led.name);
  }
  
-static void ieee80211_tx_led_activate(struct led_classdev *led_cdev)
+static int ieee80211_tx_led_activate(struct led_classdev *led_cdev)
  {
  	struct ieee80211_local *local = container_of(led_cdev->trigger,
  						     struct ieee80211_local,
  						     tx_led);
  
  	atomic_inc(&local->tx_led_active);
+
+	return 0
  }
  
  static void ieee80211_tx_led_deactivate(struct led_classdev *led_cdev)
@@ -70,13 +72,15 @@ static void ieee80211_tx_led_deactivate(struct led_classdev *led_cdev)
  	atomic_dec(&local->tx_led_active);
  }
  
-static void ieee80211_rx_led_activate(struct led_classdev *led_cdev)
+static int ieee80211_rx_led_activate(struct led_classdev *led_cdev)
  {
  	struct ieee80211_local *local = container_of(led_cdev->trigger,
  						     struct ieee80211_local,
  						     rx_led);
  
  	atomic_inc(&local->rx_led_active);
+
+	return 0;
  }
  
  static void ieee80211_rx_led_deactivate(struct led_classdev *led_cdev)
@@ -88,13 +92,15 @@ static void ieee80211_rx_led_deactivate(struct led_classdev *led_cdev)
  	atomic_dec(&local->rx_led_active);
  }
  
-static void ieee80211_assoc_led_activate(struct led_classdev *led_cdev)
+static int ieee80211_assoc_led_activate(struct led_classdev *led_cdev)
  {
  	struct ieee80211_local *local = container_of(led_cdev->trigger,
  						     struct ieee80211_local,
  						     assoc_led);
  
  	atomic_inc(&local->assoc_led_active);
+
+	return 0;
  }
  
  static void ieee80211_assoc_led_deactivate(struct led_classdev *led_cdev)
@@ -106,13 +112,15 @@ static void ieee80211_assoc_led_deactivate(struct led_classdev *led_cdev)
  	atomic_dec(&local->assoc_led_active);
  }
  
-static void ieee80211_radio_led_activate(struct led_classdev *led_cdev)
+static int ieee80211_radio_led_activate(struct led_classdev *led_cdev)
  {
  	struct ieee80211_local *local = container_of(led_cdev->trigger,
  						     struct ieee80211_local,
  						     radio_led);
  
  	atomic_inc(&local->radio_led_active);
+
+	return 0;
  }
  
  static void ieee80211_radio_led_deactivate(struct led_classdev *led_cdev)
@@ -124,13 +132,15 @@ static void ieee80211_radio_led_deactivate(struct led_classdev *led_cdev)
  	atomic_dec(&local->radio_led_active);
  }
  
-static void ieee80211_tpt_led_activate(struct led_classdev *led_cdev)
+static int ieee80211_tpt_led_activate(struct led_classdev *led_cdev)
  {
  	struct ieee80211_local *local = container_of(led_cdev->trigger,
  						     struct ieee80211_local,
  						     tpt_led);
  
  	atomic_inc(&local->tpt_led_active);
+
+	return 0;
  }
  
  static void ieee80211_tpt_led_deactivate(struct led_classdev *led_cdev)
diff --git a/net/rfkill/core.c b/net/rfkill/core.c
index a7a4e6ff9be2..1355f5ca8d22 100644
--- a/net/rfkill/core.c
+++ b/net/rfkill/core.c
@@ -141,13 +141,15 @@ static void rfkill_led_trigger_event(struct rfkill *rfkill)
  		led_trigger_event(trigger, LED_FULL);
  }
  
-static void rfkill_led_trigger_activate(struct led_classdev *led)
+static int rfkill_led_trigger_activate(struct led_classdev *led)
  {
  	struct rfkill *rfkill;
  
  	rfkill = container_of(led->trigger, struct rfkill, led_trigger);
  
  	rfkill_led_trigger_event(rfkill);
+
+	return 0;
  }
  
  const char *rfkill_get_led_trigger_name(struct rfkill *rfkill)


I fixed my branch at

	https://git.pengutronix.de/git/ukl/linux led-trigger

and wonder about how to proceed? Do you fixup v6 patch accordingly,
should I send a v7? If the latter, only this single patch or the whole
series? Or do you want to pull?

Please send v7, containing all the v6 patches. Let's add also
relevant net maintainers on CC, as well as linux-kernel list.

If there are some more that slipped through my build tests and grep
patterns I tend to go back to introducing a new member for the
conversion to be able to get the patches in and be able to revert a
single patch when another problem appears without having to throw out
the whole series.

I'm in favor of the current approach. Problems with collecting acks
for can drivers show that there is a risk we will be left with two
activation ops for a long time.

--
Best regards,
Jacek Anaszewski






