Re: [Intel-wired-lan] [PATCH v6 01/11] x86/tsc: Add base clock properties in clocksource structure
> -Original Message- > From: Thomas Gleixner > Sent: Thursday, April 11, 2024 2:51 AM > To: D, Lakshmi Sowjanya ; > jstu...@google.com; giome...@enneenne.com; cor...@lwn.net; linux- > ker...@vger.kernel.org > Cc: x...@kernel.org; net...@vger.kernel.org; linux-...@vger.kernel.org; intel- > wired-...@lists.osuosl.org; andriy.shevche...@linux.intel.com; Dong, Eddie > ; Hall, Christopher S ; > Brandeburg, Jesse ; da...@davemloft.net; > alexandre.tor...@foss.st.com; joab...@synopsys.com; > mcoquelin.st...@gmail.com; pe...@perex.cz; linux-so...@vger.kernel.org; > Nguyen, Anthony L ; > peter.hil...@opensynergy.com; N, Pandith ; Mohan, > Subramanian ; T R, Thejesh Reddy > ; D, Lakshmi Sowjanya > > Subject: Re: [PATCH v6 01/11] x86/tsc: Add base clock properties in > clocksource > structure > > On Wed, Apr 10 2024 at 17:18, lakshmi.sowjany...@intel.com wrote: > > From: Lakshmi Sowjanya D > > > > Add base clock hardware abstraction in clocksource structure. > > > > Add clocksource ID for x86 ART(Always Running Timer). The newly added > > clocksource ID and conversion parameters are used to convert time in a > > clocksource domain to base clock and vice versa. > > > > Convert the base clock to the system clock using convert_base_to_cs() > > in get_device_system_crosststamp(). > > In https://lore.kernel.org/all/875xxhi1ty.ffs@tglx I asked you to provide a > change log which explains the WHY and not the WHAT. The new change log still > fails to explain WHY this change is needed and which problem it is trying to > solve. Rephrased the commit message to: " Add base clock hardware abstraction in clocksource structure. The core code has a mechanism to convert the ART base clock to the corresponding tsc value. Provide the generic functionality in convert_base_to_cs() to convert base clock timestamps to system clocksource without requiring architecture specific parameters. Add the infrastructure in get_device_system_crosststamp()." > > I further asked you to do: > > 1) Add the clocksource_base struct and provide the infrastructure in >get_device_system_crosststamp() > > 2) Make TSC/ART use it > > But this still does #1 and #2 in one go. > > If you don't understand my review comments, then please ask. If you disagree > with them then please tell me and argue with me. > > Just ignoring them is not an option. Sorry Thomas, that was a mis-understanding. We had split only realtime as separate patch. We will split the first patch as suggested. 1. Timekeeping part(convert_base_to_cs() and infrastructure in get_device_system_crosststamp()) 2. x86(TSC/ART values update into the structure) Thanks, Sowjanya > > Thanks, > > tglx
Re: [Intel-wired-lan] [PATCH v6 01/11] x86/tsc: Add base clock properties in clocksource structure
> -Original Message-
> From: Thomas Gleixner
> Sent: Thursday, April 11, 2024 3:03 AM
> To: D, Lakshmi Sowjanya ;
> jstu...@google.com; giome...@enneenne.com; cor...@lwn.net; linux-
> ker...@vger.kernel.org
> Cc: x...@kernel.org; net...@vger.kernel.org; linux-...@vger.kernel.org; intel-
> wired-...@lists.osuosl.org; andriy.shevche...@linux.intel.com; Dong, Eddie
> ; Hall, Christopher S ;
> Brandeburg, Jesse ; da...@davemloft.net;
> alexandre.tor...@foss.st.com; joab...@synopsys.com;
> mcoquelin.st...@gmail.com; pe...@perex.cz; linux-so...@vger.kernel.org;
> Nguyen, Anthony L ;
> peter.hil...@opensynergy.com; N, Pandith ; Mohan,
> Subramanian ; T R, Thejesh Reddy
> ; D, Lakshmi Sowjanya
>
> Subject: Re: [PATCH v6 01/11] x86/tsc: Add base clock properties in
> clocksource
> structure
>
> On Wed, Apr 10 2024 at 17:18, lakshmi.sowjany...@intel.com wrote:
> > @@ -48,6 +49,7 @@ struct module;
> > * @archdata: Optional arch-specific data
> > * @max_cycles:Maximum safe cycle value which won't
> overflow on
> > * multiplication
> > + * @freq_khz: Clocksource frequency in khz.
> > * @name: Pointer to clocksource name
> > * @list: List head for registration (internal)
> > * @rating:Rating value for selection (higher is better)
> > @@ -70,6 +72,8 @@ struct module;
> > * validate the clocksource from which the snapshot was
> > * taken.
> > * @flags: Flags describing special properties
> > + * @base: Hardware abstraction for clock on which a clocksource
> > + * is based
> > * @enable:Optional function to enable the clocksource
> > * @disable: Optional function to disable the clocksource
> > * @suspend: Optional suspend function for the clocksource
> > @@ -105,12 +109,14 @@ struct clocksource {
> > struct arch_clocksource_data archdata; #endif
> > u64 max_cycles;
> > + u32 freq_khz;
>
> Q: Why is this a bad place to add this member?
>
> A: Because it creates a 4 byte hole in the data structure.
>
> > const char *name;
> > struct list_headlist;
>
> While adding it here fills a 4 byte hole.
>
> Hint:
>
> pahole -c clocksource kernel/time/clocksource.o
>
> would have told you that.
>
> > int rating;
> > enum clocksource_idsid;
> > enum vdso_clock_modevdso_clock_mode;
> > unsigned long flags;
> > + struct clocksource_base *base;
>
> > diff --git a/kernel/time/timekeeping.c b/kernel/time/timekeeping.c
> > index b58dffc58a8f..2542cfefbdee 100644
> > --- a/kernel/time/timekeeping.c
> > +++ b/kernel/time/timekeeping.c
> > @@ -1193,6 +1193,40 @@ static bool timestamp_in_interval(u64 start, u64
> end, u64 ts)
> > return false;
> > }
> >
> > +static bool convert_clock(u64 *val, u32 numerator, u32 denominator) {
> > + u64 rem, res;
> > +
> > + if (!numerator || !denominator)
> > + return false;
> > +
> > + res = div64_u64_rem(*val, denominator, &rem) * numerator;
> > + *val = res + div_u64(rem * numerator, denominator);
> > + return true;
> > +}
> > +
> > +static bool convert_base_to_cs(struct system_counterval_t *scv) {
> > + struct clocksource *cs = tk_core.timekeeper.tkr_mono.clock;
> > + struct clocksource_base *base = cs->base;
> > + u32 num, den;
> > +
> > + /* The timestamp was taken from the time keeper clock source */
> > + if (cs->id == scv->cs_id)
> > + return true;
> > +
> > + /* Check whether cs_id matches the base clock */
> > + if (!base || base->id != scv->cs_id)
> > + return false;
> > +
> > + num = scv->use_nsecs ? cs->freq_khz : base->numerator;
> > + den = scv->use_nsecs ? USEC_PER_SEC : base->denominator;
> > +
> > + convert_clock(&scv->cycles, num, den);
>
> Q: Why does this ignore the return value of convert_clock() ?
>
> A: Because all drivers will correctly fill in everything.
>
> Q: Then why does convert_clock() bother to check and have a return
>value?
>
> A: Because drivers will fail to correctly fill in everything
Agreed.
Will add a check for error case:
if (!convert_clock(&scv->cycles, num, den))
return false;
Thanks,
Sowjanya
>
> Thanks,
>
> tglx
Re: [Intel-wired-lan] [PATCH v6 08/11] timekeeping: Add function to convert realtime to base clock
> -Original Message-
> From: Thomas Gleixner
> Sent: Thursday, April 11, 2024 3:46 AM
> To: D, Lakshmi Sowjanya ;
> jstu...@google.com; giome...@enneenne.com; cor...@lwn.net; linux-
> ker...@vger.kernel.org
> Cc: x...@kernel.org; net...@vger.kernel.org; linux-...@vger.kernel.org; intel-
> wired-...@lists.osuosl.org; andriy.shevche...@linux.intel.com; Dong, Eddie
> ; Hall, Christopher S ;
> Brandeburg, Jesse ; da...@davemloft.net;
> alexandre.tor...@foss.st.com; joab...@synopsys.com;
> mcoquelin.st...@gmail.com; pe...@perex.cz; linux-so...@vger.kernel.org;
> Nguyen, Anthony L ;
> peter.hil...@opensynergy.com; N, Pandith ; Mohan,
> Subramanian ; T R, Thejesh Reddy
> ; D, Lakshmi Sowjanya
>
> Subject: Re: [PATCH v6 08/11] timekeeping: Add function to convert realtime to
> base clock
>
> On Wed, Apr 10 2024 at 17:18, lakshmi.sowjany...@intel.com wrote:
> > From: Lakshmi Sowjanya D
> >
> > PPS(Pulse Per Second) generates signals in realtime, but Timed IO
>
> ... generates signals based on CLOCK_REALTIME, but ...
>
> > hardware understands time in base clock reference.
>
> The hardware does not understand anything.
>
> > Add an interface,
> > ktime_real_to_base_clock() to convert realtime to base clock.
> >
> > Add the helper function timekeeping_clocksource_has_base(), to check
> > whether the current clocksource has the same base clock. This will be
> > used by Timed IO device to check if the base clock is X86_ART(Always
> > Running Timer).
>
> Again this fails to explain the rationale and as this is a core change which
> is
> hardware agnostic the whole Timed IO and ART reference is not really helpful.
> Something like this:
>
> "PPS (Pulse Per Second) generates a hardware pulse every second based
>on CLOCK_REALTIME. This works fine when the pulse is generated in
>software from a hrtimer callback function.
>
>For hardware which generates the pulse by programming a timer it's
>required to convert CLOCK_REALTIME to the underlying hardware clock.
>
>The X86 Timed IO device is based on the Always Running Timer (ART),
>which is the base clock of the TSC, which is usually the system
>clocksource on X86.
>
>The core code already has functionality to convert base clock
>timestamps to system clocksource timestamps, but there is no support
>for converting the other way around.
>
>Provide the required functionality to support such devices in a
>generic way to avoid code duplication in drivers:
>
> 1) ktime_real_to_base_clock() to convert a CLOCK_REALTIME
> timestamp to a base clock timestamp
>
> 2) timekeeping_clocksource_has_base() to allow drivers to validate
> that the system clocksource is based on a particular
> clocksource ID.
Thanks for the commit message.
I will update as suggested.
>
> > +static bool convert_cs_to_base(u64 *cycles, enum clocksource_ids
> > +base_id) {
> > + struct clocksource *cs = tk_core.timekeeper.tkr_mono.clock;
> > + struct clocksource_base *base = cs->base;
> > +
> > + /* Check whether base_id matches the base clock */
> > + if (!base || base->id != base_id)
> > + return false;
> > +
> > + *cycles -= base->offset;
> > + if (!convert_clock(cycles, base->denominator, base->numerator))
> > + return false;
> > + return true;
> > +}
> > +
> > +static u64 convert_ns_to_cs(u64 delta) {
> > + struct tk_read_base *tkr = &tk_core.timekeeper.tkr_mono;
> > +
> > + return div_u64((delta << tkr->shift) - tkr->xtime_nsec, tkr->mult);
> > +}
>
> > +bool ktime_real_to_base_clock(ktime_t treal, enum clocksource_ids
> > +base_id, u64 *cycles)
>
> As this is a kernel API function it really wants kernel-doc comment to
> explain the
> functionality, the arguments and the return value.
Will add the following documentation:
" ktime_real_to_base_clock()- Convert CLOCK_REALTIME timestamp to a base clock
timestamp.
@treal: CLOCK_REALTIME timestamp to convert.
@base_id: base clocksource id.
@*cycles: pointer to store the converted base clock timestamp.
Converts a supplied, future realtime clock value to the corresponding base
clock value.
Return: true if the conversion is successful, false otherwise."
>
> > +{
> > + struct timekeeper *tk = &tk_core.timekeeper;
> > + unsigned int seq;
> > + u64 delta;
> > +
> > + do {
> > + seq = read_seqcount_begin(&tk_core.seq);
> > + if ((u64)treal < tk->tkr_mono.base_real)
> > + return false;
> > + delta = (u64)treal - tk->tkr_mono.base_real;
>
> In the previous version you had a sanity check on delta:
>
> >>> + if (delta > tk->tkr_mono.clock->max_idle_ns)
> >>> + return false;
>
> And I told you:
>
> >> I don't think this cutoff is valid. There is no guarantee that this
> >> is linear unless:
> >>
> >> Treal[last timekeeper update] <= treal < Treal[next timekeeper
> >> update]
> >>
> >> Look at the danc
Re: [Intel-wired-lan] [PATCH v6 09/11] pps: generators: Add PPS Generator TIO Driver
> -Original Message-
> From: Thomas Gleixner
> Sent: Thursday, April 11, 2024 3:59 AM
> To: D, Lakshmi Sowjanya ;
> jstu...@google.com; giome...@enneenne.com; cor...@lwn.net; linux-
> ker...@vger.kernel.org
> Cc: x...@kernel.org; net...@vger.kernel.org; linux-...@vger.kernel.org; intel-
> wired-...@lists.osuosl.org; andriy.shevche...@linux.intel.com; Dong, Eddie
> ; Hall, Christopher S ;
> Brandeburg, Jesse ; da...@davemloft.net;
> alexandre.tor...@foss.st.com; joab...@synopsys.com;
> mcoquelin.st...@gmail.com; pe...@perex.cz; linux-so...@vger.kernel.org;
> Nguyen, Anthony L ;
> peter.hil...@opensynergy.com; N, Pandith ; Mohan,
> Subramanian ; T R, Thejesh Reddy
> ; D, Lakshmi Sowjanya
>
> Subject: Re: [PATCH v6 09/11] pps: generators: Add PPS Generator TIO Driver
>
> On Wed, Apr 10 2024 at 17:18, lakshmi.sowjany...@intel.com wrote:
> > +static bool pps_generate_next_pulse(struct pps_tio *tio, ktime_t
> > +expires) {
> > + u64 art;
> > +
> > + if (!ktime_real_to_base_clock(expires, CSID_X86_ART, &art)) {
> > + pps_tio_disable(tio);
> > + return false;
> > + }
> > +
> > + pps_compv_write(tio, art - ART_HW_DELAY_CYCLES);
> > + return true;
> > +}
> > +
> > +static enum hrtimer_restart hrtimer_callback(struct hrtimer *timer) {
> > + struct pps_tio *tio = container_of(timer, struct pps_tio, timer);
> > + ktime_t expires, now;
> > + u32 event_count;
> > +
> > + guard(spinlock)(&tio->lock);
> > +
> > + /* Check if any event is missed. If an event is missed, TIO will be
> disabled*/
> > + event_count = pps_tio_read(tio, TIOEC);
> > + if (!tio->prev_count && tio->prev_count == event_count)
> > + goto err;
> > + tio->prev_count = event_count;
> > + expires = hrtimer_get_expires(timer);
> > + now = ktime_get_real();
> > +
> > + if (now - expires < SAFE_TIME_NS) {
> > + if (!pps_generate_next_pulse(tio, expires + SAFE_TIME_NS))
> > + goto err;
> > + }
>
> If the hrtimer callback is invoked late so that now - expires is >=
> SAFE_TIME_NS
> then this just forwards the timer and tries again.
Yes we will introduce a return HRTIMER_NORESTART if the time is expired.
>
> This lacks any form of explanation why this is correct as obviously there
> will be a
> missing pulse, no?
We had added an event count check to detect the missed pulse(i.e if we had
programmed an expired time).
Timed I/O hardware has an event count register to log the number of pulses
generated.
>
> > + hrtimer_forward(timer, now, NSEC_PER_SEC / 2);
> > + return HRTIMER_RESTART;
> > +err:
> > + dev_err(tio->dev, "Event missed, Disabling Timed I/O");
> > + pps_tio_disable(tio);
>
> Why does this disable it again? The failure path in
> pps_generate_next_pulse() does so already, no?
will remove disabling twice in the next version of patchset.
>
> > + return HRTIMER_NORESTART;
> > +}
> > +
>
> Thanks,
>
> tglx
Re: [Intel-wired-lan] [PATCH iwl-next] ice: Deduplicate tc action setup
On Mon, Apr 15, 2024 at 10:49:07AM +0200, Marcin Szycik wrote: > ice_tc_setup_redirect_action() and ice_tc_setup_mirror_action() are almost > identical, except for setting filter action. Reduce them to one function > with an extra param, which handles both cases. > > Reviewed-by: Mateusz Polchlopek > Signed-off-by: Marcin Szycik > --- > drivers/net/ethernet/intel/ice/ice_tc_lib.c | 56 ++--- > 1 file changed, 15 insertions(+), 41 deletions(-) Less is more :) Reviewed-by: Simon Horman
Re: [Intel-wired-lan] [iwl-next v3 3/7] ice: add basic devlink subfunctions support
Tue, Apr 16, 2024 at 08:16:17AM CEST, michal.swiatkow...@linux.intel.com wrote:
>On Tue, Apr 16, 2024 at 07:14:43AM +0200, Michal Swiatkowski wrote:
>> On Mon, Apr 15, 2024 at 11:10:50AM +0200, Jiri Pirko wrote:
>> > Mon, Apr 15, 2024 at 10:39:39AM CEST, michal.swiatkow...@linux.intel.com
>> > wrote:
>> > >On Fri, Apr 12, 2024 at 09:12:18AM +0200, Jiri Pirko wrote:
>> > >> Fri, Apr 12, 2024 at 08:30:49AM CEST,
>> > >> michal.swiatkow...@linux.intel.com wrote:
>> > >> >From: Piotr Raczynski
>> >
>> > [...]
>> >
>> > >> >+static int
>> > >> >+ice_devlink_port_fn_state_get(struct devlink_port *port,
>> > >> >+enum devlink_port_fn_state *state,
>> > >> >+enum devlink_port_fn_opstate *opstate,
>> > >> >+struct netlink_ext_ack *extack)
>> > >> >+{
>> > >> >+ struct ice_dynamic_port *dyn_port;
>> > >> >+
>> > >> >+ dyn_port = ice_devlink_port_to_dyn(port);
>> > >> >+
>> > >> >+ if (dyn_port->active) {
>> > >> >+ *state = DEVLINK_PORT_FN_STATE_ACTIVE;
>> > >> >+ *opstate = DEVLINK_PORT_FN_OPSTATE_ATTACHED;
>> > >>
>> > >> Interesting. This means that you don't distinguish between admin state
>> > >> and operational state. Meaning, when user does activate, you atomically
>> > >> achive the hw attachment and it is ready to go before activation cmd
>> > >> returns, correct? I'm just making sure I understand the code.
>> > >>
>> > >
>> > >I am setting the dyn_port->active after the activation heppens, so it is
>> > >true, when active is set it is ready to go.
>> > >
>> > >Do you mean that dyn_port->active should be set even before the
>> > >activation is
>> > >finished? I mean when user only call devlink to active the port?
>> >
>> > The devlink instance lock is taken the whole time, isn't it?
>> >
>>
>> I don't take PF devlink lock here. Only subfunction devlink lock is
>> taken during the initialization of subfunction.
>>
>
>Did you mean that the devlink lock is taken for DEVLINK_CMD_PORT_SET/GET
>command? In this case, I think so, it is for the whole time of the
>command execution.
Yes.
>
>Sorry I probably missed the point.
Np.
>
>> > >
>> > >>
>> > >> >+ } else {
>> > >> >+ *state = DEVLINK_PORT_FN_STATE_INACTIVE;
>> > >> >+ *opstate = DEVLINK_PORT_FN_OPSTATE_DETACHED;
>> > >> >+ }
>> > >> >+
>> > >> >+ return 0;
>> > >> >+}
>> > >> >+
>> >
>> > [...]
[Intel-wired-lan] [PATCH iwl-net] ice: Interpret .set_channels() input differently
A bug occurs because a safety check guarding AF_XDP-related queues in
ethnl_set_channels(), does not trigger. This happens, because kernel and
ice driver interpret the ethtool command differently.
How the bug occurs:
1. ethtool -l -> combined: 40
2. Attach AF_XDP to queue 30
3. ethtool -L rx 15 tx 15
combined number is not specified, so command becomes {rx_count = 15,
tx_count = 15, combined_count = 40}.
4. ethnl_set_channels checks, if there are any AF_XDP of queues from the
new (combined_count + rx_count) to the old one, so from 55 to 40, check
does not trigger.
5. ice interprets `rx 15 tx 15` as 15 combined channels and deletes the
queue that AF_XDP is attached to.
Interpret the command in a way that is more consistent with ethtool
manual [0] (--show-channels and --set-channels).
Considering that in the ice driver only the difference between RX and TX
queues forms dedicated channels, change the correct way to set number of
channels to:
ethtool -L combined 10 /* For symmetric queues */
ethtool -L combined 8 tx 2 rx 0 /* For asymmetric queues */
[0] https://man7.org/linux/man-pages/man8/ethtool.8.html
Fixes: 87324e747fde ("ice: Implement ethtool ops for channels")
Reviewed-by: Michal Swiatkowski
Signed-off-by: Larysa Zaremba
---
drivers/net/ethernet/intel/ice/ice_ethtool.c | 22 ++--
1 file changed, 6 insertions(+), 16 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c
b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index 78b833b3e1d7..d91f41f61bce 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -3593,7 +3593,6 @@ static int ice_set_channels(struct net_device *dev,
struct ethtool_channels *ch)
struct ice_pf *pf = vsi->back;
int new_rx = 0, new_tx = 0;
bool locked = false;
- u32 curr_combined;
int ret = 0;
/* do not support changing channels in Safe Mode */
@@ -3615,22 +3614,13 @@ static int ice_set_channels(struct net_device *dev,
struct ethtool_channels *ch)
return -EOPNOTSUPP;
}
- curr_combined = ice_get_combined_cnt(vsi);
+ if (!ch->combined_count) {
+ netdev_err(dev, "Please specify at least 1 combined channel\n");
+ return -EINVAL;
+ }
- /* these checks are for cases where user didn't specify a particular
-* value on cmd line but we get non-zero value anyway via
-* get_channels(); look at ethtool.c in ethtool repository (the user
-* space part), particularly, do_schannels() routine
-*/
- if (ch->rx_count == vsi->num_rxq - curr_combined)
- ch->rx_count = 0;
- if (ch->tx_count == vsi->num_txq - curr_combined)
- ch->tx_count = 0;
- if (ch->combined_count == curr_combined)
- ch->combined_count = 0;
-
- if (!(ch->combined_count || (ch->rx_count && ch->tx_count))) {
- netdev_err(dev, "Please specify at least 1 Rx and 1 Tx
channel\n");
+ if (ch->rx_count && ch->tx_count) {
+ netdev_err(dev, "Dedicated RX or TX channels cannot be used
simultaneously\n");
return -EINVAL;
}
--
2.43.0
Re: [Intel-wired-lan] [PATCH iwl-net v2] igc: Fix deadlock on module removal
On Mon, Apr 15, 2024 at 12:59:37PM +0200, Kurt Kanzenbach wrote:
> From: Lukas Wunner
>
> The removal of the igc module leads to a deadlock:
>
> |[Mon Apr 8 17:38:55 2024] __mutex_lock.constprop.0+0x3e5/0x7a0
> |[Mon Apr 8 17:38:55 2024] ? preempt_count_add+0x85/0xd0
> |[Mon Apr 8 17:38:55 2024] __mutex_lock_slowpath+0x13/0x20
> |[Mon Apr 8 17:38:55 2024] mutex_lock+0x3b/0x50
> |[Mon Apr 8 17:38:55 2024] rtnl_lock+0x19/0x20
> |[Mon Apr 8 17:38:55 2024] unregister_netdevice_notifier+0x2a/0xc0
> |[Mon Apr 8 17:38:55 2024] netdev_trig_deactivate+0x25/0x70
> |[Mon Apr 8 17:38:55 2024] led_trigger_set+0xe2/0x2d0
> |[Mon Apr 8 17:38:55 2024] led_classdev_unregister+0x4f/0x100
> |[Mon Apr 8 17:38:55 2024] devm_led_classdev_release+0x15/0x20
> |[Mon Apr 8 17:38:55 2024] release_nodes+0x47/0xc0
> |[Mon Apr 8 17:38:55 2024] devres_release_all+0x9f/0xe0
> |[Mon Apr 8 17:38:55 2024] device_del+0x272/0x3c0
> |[Mon Apr 8 17:38:55 2024] netdev_unregister_kobject+0x8c/0xa0
> |[Mon Apr 8 17:38:55 2024] unregister_netdevice_many_notify+0x530/0x7c0
> |[Mon Apr 8 17:38:55 2024] unregister_netdevice_queue+0xad/0xf0
> |[Mon Apr 8 17:38:55 2024] unregister_netdev+0x21/0x30
> |[Mon Apr 8 17:38:55 2024] igc_remove+0xfb/0x1f0 [igc]
> |[Mon Apr 8 17:38:55 2024] pci_device_remove+0x42/0xb0
> |[Mon Apr 8 17:38:55 2024] device_remove+0x43/0x70
>
> unregister_netdev() acquires the RNTL lock and releases the LEDs bound
> to that netdevice. However, netdev_trig_deactivate() and later
> unregister_netdevice_notifier() try to acquire the RTNL lock again.
>
> Avoid this situation by not using the device-managed LED class
> functions.
>
> Link:
> https://lore.kernel.org/r/CAEhC_B=ksywxcg_+aqqxurgegkq+4mqnsv8ebhokbc3-obj...@mail.gmail.com/
> Link: https://lore.kernel.org/r/ZhRD3cOtz5i-61PB@mail-itl/
> Reported-by: Roman Lozko
> Reported-by: "Marek Marczykowski-Górecki"
> Fixes: ea578703b03d ("igc: Add support for LEDs on i225/i226")
> Signed-off-by: Lukas Wunner
> [Kurt: Wrote commit message and tested on i225]
> Signed-off-by: Kurt Kanzenbach
I am aware that this patch seems to have also been submitted by Lucas
himself. I'd like to suggest that we focus on review of that submission.
https://lore.kernel.org/netdev/2f1be6b1cf2b3346929b0049f2ac7d7d79acb5c9.1713188539.git.lu...@wunner.de/
Re: [Intel-wired-lan] [PATCH net] igc: Fix LED-related deadlock on driver unbind
On Mon, Apr 15, 2024 at 03:48:48PM +0200, Lukas Wunner wrote:
> Roman reports a deadlock on unplug of a Thunderbolt docking station
> containing an Intel I225 Ethernet adapter.
>
> The root cause is that led_classdev's for LEDs on the adapter are
> registered such that they're device-managed by the netdev. That
> results in recursive acquisition of the rtnl_lock() mutex on unplug:
>
> When the driver calls unregister_netdev(), it acquires rtnl_lock(),
> then frees the device-managed resources. Upon unregistering the LEDs,
> netdev_trig_deactivate() invokes unregister_netdevice_notifier(),
> which tries to acquire rtnl_lock() again.
>
> Avoid by using non-device-managed LED registration.
>
> Stack trace for posterity:
>
> schedule+0x6e/0xf0
> schedule_preempt_disabled+0x15/0x20
> __mutex_lock+0x2a0/0x750
> unregister_netdevice_notifier+0x40/0x150
> netdev_trig_deactivate+0x1f/0x60 [ledtrig_netdev]
> led_trigger_set+0x102/0x330
> led_classdev_unregister+0x4b/0x110
> release_nodes+0x3d/0xb0
> devres_release_all+0x8b/0xc0
> device_del+0x34f/0x3c0
> unregister_netdevice_many_notify+0x80b/0xaf0
> unregister_netdev+0x7c/0xd0
> igc_remove+0xd8/0x1e0 [igc]
> pci_device_remove+0x3f/0xb0
>
> Fixes: ea578703b03d ("igc: Add support for LEDs on i225/i226")
> Reported-by: Roman Lozko
> Closes:
> https://lore.kernel.org/r/CAEhC_B=ksywxcg_+aqqxurgegkq+4mqnsv8ebhokbc3-obj...@mail.gmail.com/
> Signed-off-by: Kurt Kanzenbach
> Signed-off-by: Lukas Wunner
> Cc: Heiner Kallweit
I am aware that Kurt has submitted what appears to be the same patch [1,2],
which I'm inclined to put down to miscommunication (email based workflows
are like that sometimes).
FWIIW, it is my understanding is that the patch originated from
Lukas[3], and thus it seems most appropriate to take his submission.
As for the patch itself, I agree that it addresses the problem at hand.
For the record, I have not tested it.
Reviewed-by: Simon Horman
[1] [PATCH iwl-net] igc: Fix deadlock on module removal
https://lore.kernel.org/netdev/20240411-igc_led_deadlock-v1-1-0da98a3c6...@linutronix.de/
[2] [PATCH iwl-net v2] igc: Fix deadlock on module removal
https://lore.kernel.org/netdev/20240411-igc_led_deadlock-v2-1-b758c0c88...@linutronix.de/
[3] Re: Deadlock in pciehp on dock disconnect
https://lore.kernel.org/all/zhbn9p1yoycix...@wunner.de/
Re: [Intel-wired-lan] [PATCH net] igc: Fix LED-related deadlock on driver unbind
On Mon Apr 15 2024, Lukas Wunner wrote:
> Roman reports a deadlock on unplug of a Thunderbolt docking station
> containing an Intel I225 Ethernet adapter.
>
> The root cause is that led_classdev's for LEDs on the adapter are
> registered such that they're device-managed by the netdev. That
> results in recursive acquisition of the rtnl_lock() mutex on unplug:
>
> When the driver calls unregister_netdev(), it acquires rtnl_lock(),
> then frees the device-managed resources. Upon unregistering the LEDs,
> netdev_trig_deactivate() invokes unregister_netdevice_notifier(),
> which tries to acquire rtnl_lock() again.
>
> Avoid by using non-device-managed LED registration.
>
> Stack trace for posterity:
>
> schedule+0x6e/0xf0
> schedule_preempt_disabled+0x15/0x20
> __mutex_lock+0x2a0/0x750
> unregister_netdevice_notifier+0x40/0x150
> netdev_trig_deactivate+0x1f/0x60 [ledtrig_netdev]
> led_trigger_set+0x102/0x330
> led_classdev_unregister+0x4b/0x110
> release_nodes+0x3d/0xb0
> devres_release_all+0x8b/0xc0
> device_del+0x34f/0x3c0
> unregister_netdevice_many_notify+0x80b/0xaf0
> unregister_netdev+0x7c/0xd0
> igc_remove+0xd8/0x1e0 [igc]
> pci_device_remove+0x3f/0xb0
>
> Fixes: ea578703b03d ("igc: Add support for LEDs on i225/i226")
> Reported-by: Roman Lozko
> Closes:
> https://lore.kernel.org/r/CAEhC_B=ksywxcg_+aqqxurgegkq+4mqnsv8ebhokbc3-obj...@mail.gmail.com/
> Signed-off-by: Kurt Kanzenbach
I think, the first SoB has to be yours, because you are the patch
author. In fact, my SoB is not required at all.
However, feel free to add:
Reviewed-by: Kurt Kanzenbach
Tested-by: Kurt Kanzenbach # Intel i225
Thanks,
Kurt
signature.asc
Description: PGP signature
[Intel-wired-lan] [PATCH iwl-next] iavf: flower: validate control flags
This driver currently doesn't support any control flags.
Use flow_rule_has_control_flags() to check for control flags,
such as can be set through `tc flower ... ip_flags frag`.
In case any control flags are masked, flow_rule_has_control_flags()
sets a NL extended error message, and we return -EOPNOTSUPP.
Only compile-tested.
Signed-off-by: Asbjørn Sloth Tønnesen
---
drivers/net/ethernet/intel/iavf/iavf_main.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/net/ethernet/intel/iavf/iavf_main.c
b/drivers/net/ethernet/intel/iavf/iavf_main.c
index 13361a780ece..f14355d52f47 100644
--- a/drivers/net/ethernet/intel/iavf/iavf_main.c
+++ b/drivers/net/ethernet/intel/iavf/iavf_main.c
@@ -3757,6 +3757,10 @@ static int iavf_parse_cls_flower(struct iavf_adapter
*adapter,
flow_rule_match_control(rule, &match);
addr_type = match.key->addr_type;
+
+ if (flow_rule_has_control_flags(match.mask->flags,
+ f->common.extack))
+ return -EOPNOTSUPP;
}
if (addr_type == FLOW_DISSECTOR_KEY_IPV4_ADDRS) {
--
2.43.0
[Intel-wired-lan] [PATCH iwl-next] i40e: flower: validate control flags
This driver currently doesn't support any control flags.
Use flow_rule_has_control_flags() to check for control flags,
such as can be set through `tc flower ... ip_flags frag`.
In case any control flags are masked, flow_rule_has_control_flags()
sets a NL extended error message, and we return -EOPNOTSUPP.
Only compile-tested.
Signed-off-by: Asbjørn Sloth Tønnesen
---
drivers/net/ethernet/intel/i40e/i40e_main.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/net/ethernet/intel/i40e/i40e_main.c
b/drivers/net/ethernet/intel/i40e/i40e_main.c
index 0bdcdea0be3e..e219f757820d 100644
--- a/drivers/net/ethernet/intel/i40e/i40e_main.c
+++ b/drivers/net/ethernet/intel/i40e/i40e_main.c
@@ -8643,6 +8643,10 @@ static int i40e_parse_cls_flower(struct i40e_vsi *vsi,
flow_rule_match_control(rule, &match);
addr_type = match.key->addr_type;
+
+ if (flow_rule_has_control_flags(match.mask->flags,
+ f->common.extack))
+ return -EOPNOTSUPP;
}
if (addr_type == FLOW_DISSECTOR_KEY_IPV4_ADDRS) {
--
2.43.0
[Intel-wired-lan] [PATCH iwl-next] ice: flower: validate control flags
This driver currently doesn't support any control flags.
Use flow_rule_has_control_flags() to check for control flags,
such as can be set through `tc flower ... ip_flags frag`.
In case any control flags are masked, flow_rule_has_control_flags()
sets a NL extended error message, and we return -EOPNOTSUPP.
Only compile-tested.
Signed-off-by: Asbjørn Sloth Tønnesen
---
drivers/net/ethernet/intel/ice/ice_tc_lib.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/net/ethernet/intel/ice/ice_tc_lib.c
b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
index 2f2fce285ecd..361abd7d7561 100644
--- a/drivers/net/ethernet/intel/ice/ice_tc_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_tc_lib.c
@@ -1673,6 +1673,10 @@ ice_parse_cls_flower(struct net_device *filter_dev,
struct ice_vsi *vsi,
flow_rule_match_control(rule, &match);
addr_type = match.key->addr_type;
+
+ if (flow_rule_has_control_flags(match.mask->flags,
+ fltr->extack))
+ return -EOPNOTSUPP;
}
if (addr_type == FLOW_DISSECTOR_KEY_IPV4_ADDRS) {
--
2.43.0
[Intel-wired-lan] [PATCH iwl-next] igb: flower: validate control flags
This driver currently doesn't support any control flags.
Use flow_rule_match_has_control_flags() to check for control flags,
such as can be set through `tc flower ... ip_flags frag`.
In case any control flags are masked, flow_rule_match_has_control_flags()
sets a NL extended error message, and we return -EOPNOTSUPP.
Only compile-tested.
Signed-off-by: Asbjørn Sloth Tønnesen
---
drivers/net/ethernet/intel/igb/igb_main.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/drivers/net/ethernet/intel/igb/igb_main.c
b/drivers/net/ethernet/intel/igb/igb_main.c
index 74a998fcaa6f..e4c65d3819d7 100644
--- a/drivers/net/ethernet/intel/igb/igb_main.c
+++ b/drivers/net/ethernet/intel/igb/igb_main.c
@@ -2597,6 +2597,9 @@ static int igb_parse_cls_flower(struct igb_adapter
*adapter,
return -EOPNOTSUPP;
}
+ if (flow_rule_match_has_control_flags(rule, extack))
+ return -EOPNOTSUPP;
+
if (flow_rule_match_key(rule, FLOW_DISSECTOR_KEY_ETH_ADDRS)) {
struct flow_match_eth_addrs match;
--
2.43.0
Re: [Intel-wired-lan] [PATCH iwl-next v5] ice: Add automatic VF reset on Tx MDD events
> -Original Message- > From: Intel-wired-lan On Behalf Of Simon > Horman > Sent: Monday, April 8, 2024 3:32 PM > To: Marcin Szycik > Cc: Loktionov, Aleksandr ; Drewek, Wojciech > ; net...@vger.kernel.org; Chmielewski, Pawel > ; Nguyen, Anthony L > ; Wang, Liang-min ; > Kitszel, Przemyslaw ; intel-wired- > l...@lists.osuosl.org > Subject: Re: [Intel-wired-lan] [PATCH iwl-next v5] ice: Add automatic VF > reset on > Tx MDD events > > On Thu, Apr 04, 2024 at 04:04:51PM +0200, Marcin Szycik wrote: > > In cases when VF sends malformed packets that are classified as > > malicious, it can cause Tx queue to freeze as a result of Malicious > > Driver Detection event. Such malformed packets can appear as a result > > of a faulty userspace app running on VF. This frozen queue can be > > stuck for several minutes being unusable. > > > > User might prefer to immediately bring the VF back to operational > > state after such event, which can be done by automatically resetting > > the VF which caused MDD. This is already implemented for Rx events > > (mdd-auto-reset-vf flag private flag needs to be set). > > > > Extend the VF auto reset to also cover Tx MDD events. When any MDD > > event occurs on VF (Tx or Rx) and the mdd-auto-reset-vf private flag > > is set, perform a graceful VF reset to quickly bring it back to operational > > state. > > > > Reviewed-by: Wojciech Drewek > > Reviewed-by: Przemek Kitszel > > Co-developed-by: Liang-Min Wang > > Signed-off-by: Liang-Min Wang > > Signed-off-by: Marcin Szycik > > Reviewed-by: Simon Horman Tested-by: Rafal Romanowski
Re: [Intel-wired-lan] [PATCH] e1000e: move force SMBUS near the end of enable_ulp function
On Mon, 15 Apr 2024 13:15:41 +0300 Lifshits, Vitaly wrote: > Thank you for this patch and this observation. > I think that you found a real misbehaviour in the original patch. > However, I still think that forcing SMBUS functionality shouldn't be > part of the ULP enabling flow, since they are two independent > configurations. > > I will soon submit a patch where I wrap forcing SMBUS in e1000_shutdown > with an if that checks if the FWSM_FW_VALID bit it set. Why are you submitting a patch instead of asking the author to change theirs? This is not how code reviews work.
Re: [Intel-wired-lan] [PATCH net-next v2 07/15] mm: page_frag: add '_va' suffix to page_frag API
On Mon, 2024-04-15 at 21:19 +0800, Yunsheng Lin wrote: > Currently most of the API for page_frag API is returning > 'virtual address' as output or expecting 'virtual address' > as input, in order to differentiate the API handling between > 'virtual address' and 'struct page', add '_va' suffix to the > corresponding API mirroring the page_pool_alloc_va() API of > the page_pool. > > Signed-off-by: Yunsheng Lin This patch is a total waste of time. By that logic we should be renaming __get_free_pages since it essentially does the same thing. This just seems like more code changes for the sake of adding code changes rather than fixing anything. In my opinion it should be dropped from the set.
Re: [Intel-wired-lan] [PATCH net] igc: Fix LED-related deadlock on driver unbind
On Tue, Apr 16, 2024 at 04:06:49PM +0200, Kurt Kanzenbach wrote:
> On Mon Apr 15 2024, Lukas Wunner wrote:
> > Fixes: ea578703b03d ("igc: Add support for LEDs on i225/i226")
> > Reported-by: Roman Lozko
> > Closes:
> > https://lore.kernel.org/r/CAEhC_B=ksywxcg_+aqqxurgegkq+4mqnsv8ebhokbc3-obj...@mail.gmail.com/
> > Signed-off-by: Kurt Kanzenbach
>
> I think, the first SoB has to be yours, because you are the patch
> author. In fact, my SoB is not required at all.
My understanding is that the commit author must be identical to the last
Signed-off-by, so I put mine last. I've seen Stephen Rothwell send
complaints whenever he spotted commits in linux-next violating that.
I carried over the variable and function renaming you did to match
the driver's (or your) preferred style, hence the inclusion of your
Signed-off-by.
Thanks!
Lukas
Re: [Intel-wired-lan] [PATCH iwl-net] i40e: Report MFS in decimal base instead of hex
> -Original Message- > From: Intel-wired-lan On Behalf Of > Erwan Velu > Sent: Tuesday, March 19, 2024 7:17 AM > Cc: Velu, Erwan ; linux-ker...@vger.kernel.org; Eric > Dumazet ; net...@vger.kernel.org; Nguyen, > Anthony L ; intel-wired-...@lists.osuosl.org; > Jakub Kicinski ; Paolo Abeni ; David > S. Miller > Subject: [Intel-wired-lan] [PATCH iwl-net] i40e: Report MFS in decimal base > instead of hex > > If the MFS is set below the default (0x2600), a warning message is reported > like the following : > > MFS for port 1 has been set below the default: 600 > > This message is a bit confusing as the number shown here (600) is in fact an > hexa number: 0x600 = 1536 > > Without any explicit "0x" prefix, this message is read like the MFS is set to > 600 > bytes. > > MFS, as per MTUs, are usually expressed in decimal base. > > This commit reports both current and default MFS values in decimal so it's > less > confusing for end-users. > > A typical warning message looks like the following : > > MFS for port 1 (1536) has been set below the default (9728) > > Signed-off-by: Erwan Velu > --- > drivers/net/ethernet/intel/i40e/i40e_main.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) Tested-by: Tony Brelinski
Re: [Intel-wired-lan] [PATCH iwl-next v4 00/12] Add support for Rx timestamping for both ice and iavf drivers.
On 4/10/2024 5:16 AM, Mateusz Polchlopek wrote: Initially, during VF creation it registers the PTP clock in the system and negotiates with PF it's capabilities. In the meantime the PF enables the Flexible Descriptor for VF. Only this type of descriptor allows to receive Rx timestamps. Enabling virtual clock would be possible, though it would probably perform poorly due to the lack of direct time access. Enable timestamping should be done using SIOCSHWTSTAMP ioctl, e.g. hwstamp_ctl -i $VF -r 14 In order to report the timestamps to userspace, the VF extends timestamp to 40b. To support this feature the flexible descriptors and PTP part in iavf driver have been introduced. A majority of these patches are adding new kernel-doc issues (-Wall). Could you clean up the ones that are being added by these patches? Thanks, Tony
Re: [Intel-wired-lan] [PATCH v8 iwl-next 00/12] Introduce ETH56G PHY model for E825C products
On 4/12/2024 6:06 AM, Karol Kolacinski wrote: E825C products have a different PHY model than E822, E823 and E810 products. This PHY is ETH56G and its support is necessary to have functional PTP stack for E825C products. Can you clean up the new kernel-doc (-Wall) issues that these patches are introducing? Thanks, Tony
