On Wed, 10 Sep 2025 09:02:51 +0000, Loktionov, Aleksandr wrote:
[...]
>> >>
>> >> Reviewed-by: Kurt Kanzenbach <[email protected]>
>> >
>> >Thank you for the patch and for identifying this issue!
>> >
>> >I was wondering whether we could avoid failing the probe in cases
>> where
>> >igc_led_setup fails. It seems to me that a failure in the LED class
>> >functionality shouldn't prevent the device's core functionality from
>> >working properly.
>>
>> Indeed, that also makes sense.
>>
>> The behavior that igc_probe() succeeds even if igc_led_setup() fails
>> also seems good to me, as long as notifying users that igc's led
>> functionality is not available.
>>
>> >
>> > From what I understand, errors in this function are not due to
>> hardware
>> >malfunctions. Therefore, I suggest we remove the error propagation.
>> >
>> >Alternatively, if feasible, we could consider reordering the function
>> >calls so that the LED class setup occurs before the netdev
>> registration.
>> >
>>
>> I don't disagree with you, but I would like to hear Kurt and
>> Aleksandr's
>> opinion. Do you have any preference or suggestions?
>>
>> I'll revise and work on v2 if needed.
>> Thanks!
>
>Just in case /*I'm Alex*/ here are my 2cents:
> I’d treat LED setup as best‑effort and not fail probe if it errors.
>Warn once, mark LEDs unavailable, and continue. That keeps datapath
>up and avoids tricky probe unwind. If we still want to fail on LED errors,
>then either (a) fix the unwind (unregister_netdev et al.) or (b) move LED
>setup before register_netdev().
Got it, thank you for your opinion :)
>
> If LED labels depend on the netdev name, it’s fine to run LED setup after
> register_netdev().
>Since errors are non‑fatal, there’s no unwind complexity.
>
>Keep igc_led_setup() returning an error for internal visibility, but don’t
>propagate it as probe failure:
>err = igc_led_setup(adapter);
>if (err) {
> netdev_warn_once(netdev,
> "LED init failed (%d); continuing without LED support\n",
> err);
> adapter->leds_available = false;
>} else {
> adapter->leds_available = true;
>}
>
>In remove()/error paths, guard teardown:
>if (adapter->leds_available)
> igc_led_teardown(adapter);
I would like to adopt this approach, where we don't propagate led's
failure and manage its status using flag like leds_available.
I'll send v2 shortly.
>
>Keep current order but fully unwind on error:
>err = igc_led_setup(adapter);
>if (err) {
> unregister_netdev(netdev);
> /* del NAPI, free queues, etc. in reverse order */
> err = -E...;
> goto err_free;
>}
>
>With the best regards
>Alex
