Re: [Intel-wired-lan] [PATCH net-next 3/4] dpll: netlink/core: add support for pin-dpll signal phase offset/adjust
>From: Vadim Fedorenko
>Sent: Wednesday, September 27, 2023 8:09 PM
>
>On 27/09/2023 10:24, Arkadiusz Kubalewski wrote:
>> Add callback op (get) for pin-dpll phase-offset measurment.
>> Add callback ops (get/set) for pin signal phase adjustment.
>> Add min and max phase adjustment values to pin proprties.
>> Invoke get callbacks when filling up the pin details to provide user
>> with phase related attribute values.
>> Invoke phase-adjust set callback when phase-adjust value is provided for
>> pin-set request.
>>
>> Signed-off-by: Arkadiusz Kubalewski
>
>[...]
>
>> +static int
>> +dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr
>> *phase_adj_attr,
>> + struct netlink_ext_ack *extack)
>> +{
>> +struct dpll_pin_ref *ref;
>> +unsigned long i;
>> +s32 phase_adj;
>> +int ret;
>> +
>> +phase_adj = nla_get_s32(phase_adj_attr);
>> +if (phase_adj > pin->prop->phase_range.max ||
>> +phase_adj < pin->prop->phase_range.min) {
>> +NL_SET_ERR_MSG(extack, "phase adjust value not supported");
>> +return -EINVAL;
>> +}
>> +xa_for_each(&pin->dpll_refs, i, ref) {
>> +const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
>> +struct dpll_device *dpll = ref->dpll;
>> +
>> +if (!ops->phase_adjust_set)
>> +return -EOPNOTSUPP;
>
>I'm thinking about this part. We can potentially have dpll devices with
>different expectations on phase adjustments, right? And if one of them
>won't be able to adjust phase (or will fail in the next line), then
>netlink will return EOPNOTSUPP while _some_ of the devices will be
>adjusted. Doesn't look great. Can we think about different way to apply
>the change?
>
Well makes sense to me.
Does following makes sense as a fix?
We would call op for all devices which has been provided with the op.
If device has no op -> add extack error, continue
If device fails to set -> add extack error, continue
Function always returns 0.
Thank you!
Arkadiusz
>
>> +ret = ops->phase_adjust_set(pin,
>> +dpll_pin_on_dpll_priv(dpll, pin),
>> +dpll, dpll_priv(dpll), phase_adj,
>> +extack);
>> +if (ret)
>> +return ret;
>> +}
>> +__dpll_pin_change_ntf(pin);
>> +
>> +return 0;
>> +}
>> +
___
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
[Intel-wired-lan] [PATCH net-next v3 0/2] ethtool: Add link mode maps for forced speeds
The following patch set was initially a part of [1]. As the purpose of the original series was to add the support of the new hardware to the intel ice driver, the refactoring of advertised link modes mapping was extracted to a new set. The patch set adds a common mechanism for mapping Ethtool forced speeds with Ethtool supported link modes, which can be used in drivers code. [1] https://lore.kernel.org/netdev/20230823180633.2450617-1-pawel.chmielew...@intel.com Changelog: v2->v3: Fixed whitespaces, added missing line at end of file v1->v2: Fixed formatting, typo, moved declaration of iterator to loop line. Paul Greenwalt (1): ethtool: Add forced speed to supported link modes maps Pawel Chmielewski (1): ice: Refactor finding advertised link speed drivers/net/ethernet/intel/ice/ice.h | 1 + drivers/net/ethernet/intel/ice/ice_ethtool.c | 201 -- drivers/net/ethernet/intel/ice/ice_main.c | 2 + .../net/ethernet/qlogic/qede/qede_ethtool.c | 24 +-- include/linux/ethtool.h | 20 ++ net/ethtool/ioctl.c | 15 ++ 6 files changed, 178 insertions(+), 85 deletions(-) -- 2.37.3 ___ Intel-wired-lan mailing list Intel-wired-lan@osuosl.org https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
[Intel-wired-lan] [PATCH net-next v3 1/2] ethtool: Add forced speed to supported link modes maps
From: Paul Greenwalt
The need to map Ethtool forced speeds to Ethtool supported link modes is
common among drivers. To support this, add a common structure for forced
speed maps and a function to init them. This is solution was originally
introduced in commit 1d4e4ecccb11 ("qede: populate supported link modes
maps on module init") for qede driver.
ethtool_forced_speed_maps_init() should be called during driver init
with an array of struct ethtool_forced_speed_map to populate the mapping.
Definitions for maps themselves are left in the driver code, as the sets
of supported link modes may vary between the devices.
The qede driver was compile tested only.
Suggested-by: Andrew Lunn
Reviewed-by: Jacob Keller
Reviewed-by: Przemek Kitszel
Signed-off-by: Paul Greenwalt
Signed-off-by: Pawel Chmielewski
---
.../net/ethernet/qlogic/qede/qede_ethtool.c | 39 ++-
include/linux/ethtool.h | 20 ++
net/ethtool/ioctl.c | 13 +++
3 files changed, 45 insertions(+), 27 deletions(-)
diff --git a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
index 95820cf1cd6c..d3750e39d055 100644
--- a/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
+++ b/drivers/net/ethernet/qlogic/qede/qede_ethtool.c
@@ -201,14 +201,6 @@ static const char
qede_tests_str_arr[QEDE_ETHTOOL_TEST_MAX][ETH_GSTRING_LEN] = {
/* Forced speed capabilities maps */
-struct qede_forced_speed_map {
- u32 speed;
- __ETHTOOL_DECLARE_LINK_MODE_MASK(caps);
-
- const u32 *cap_arr;
- u32 arr_size;
-};
-
#define QEDE_FORCED_SPEED_MAP(value) \
{ \
.speed = SPEED_##value,\
@@ -263,28 +255,21 @@ static const u32 qede_forced_speed_10[] __initconst =
{
ETHTOOL_LINK_MODE_10baseLR4_ER4_Full_BIT,
};
-static struct qede_forced_speed_map qede_forced_speed_maps[] __ro_after_init =
{
- QEDE_FORCED_SPEED_MAP(1000),
- QEDE_FORCED_SPEED_MAP(1),
- QEDE_FORCED_SPEED_MAP(2),
- QEDE_FORCED_SPEED_MAP(25000),
- QEDE_FORCED_SPEED_MAP(4),
- QEDE_FORCED_SPEED_MAP(5),
- QEDE_FORCED_SPEED_MAP(10),
+static struct ethtool_forced_speed_map
+ qede_forced_speed_maps[] __ro_after_init = {
+ QEDE_FORCED_SPEED_MAP(1000),
+ QEDE_FORCED_SPEED_MAP(1),
+ QEDE_FORCED_SPEED_MAP(2),
+ QEDE_FORCED_SPEED_MAP(25000),
+ QEDE_FORCED_SPEED_MAP(4),
+ QEDE_FORCED_SPEED_MAP(5),
+ QEDE_FORCED_SPEED_MAP(10),
};
void __init qede_forced_speed_maps_init(void)
{
- struct qede_forced_speed_map *map;
- u32 i;
-
- for (i = 0; i < ARRAY_SIZE(qede_forced_speed_maps); i++) {
- map = qede_forced_speed_maps + i;
-
- linkmode_set_bit_array(map->cap_arr, map->arr_size, map->caps);
- map->cap_arr = NULL;
- map->arr_size = 0;
- }
+ ethtool_forced_speed_maps_init(qede_forced_speed_maps,
+ ARRAY_SIZE(qede_forced_speed_maps));
}
/* Ethtool callbacks */
@@ -564,8 +549,8 @@ static int qede_set_link_ksettings(struct net_device *dev,
const struct ethtool_link_ksettings *cmd)
{
const struct ethtool_link_settings *base = &cmd->base;
+ const struct ethtool_forced_speed_map *map;
struct qede_dev *edev = netdev_priv(dev);
- const struct qede_forced_speed_map *map;
struct qed_link_output current_link;
struct qed_link_params params;
u32 i;
diff --git a/include/linux/ethtool.h b/include/linux/ethtool.h
index 62b61527bcc4..4e2c1d07df38 100644
--- a/include/linux/ethtool.h
+++ b/include/linux/ethtool.h
@@ -1052,4 +1052,24 @@ static inline int ethtool_mm_frag_size_min_to_add(u32
val_min, u32 *val_add,
* next string.
*/
extern __printf(2, 3) void ethtool_sprintf(u8 **data, const char *fmt, ...);
+
+/* Link mode to forced speed capabilities maps */
+struct ethtool_forced_speed_map {
+ u32 speed;
+ __ETHTOOL_DECLARE_LINK_MODE_MASK(caps);
+
+ const u32 *cap_arr;
+ u32 arr_size;
+};
+
+/**
+ * ethtool_forced_speed_maps_init
+ * @maps: Pointer to an array of Ethtool forced speed map
+ * @size: Array size
+ *
+ * Initialize an array of Ethtool forced speed map to Ethtool link modes. This
+ * should be called during driver module init.
+ */
+void ethtool_forced_speed_maps_init(struct ethtool_forced_speed_map *maps,
+ u32 size);
#endif /* _LINUX_ETHTOOL_H */
diff --git a/net/ethtool/ioctl.c b/net/ethtool/ioctl.c
index 0b0ce4f81c01..34507691fc9d 100644
--- a/net/ethtool/ioctl.c
+++ b/net/ethtool/ioctl.c
@@ -3
[Intel-wired-lan] [PATCH net-next v3 2/2] ice: Refactor finding advertised link speed
Refactor ice_get_link_ksettings to using forced speed to link modes mapping.
Suggested-by : Alexander Lobakin
Reviewed-by: Jacob Keller
Reviewed-by: Przemek Kitszel
Signed-off-by: Paul Greenwalt
Signed-off-by: Pawel Chmielewski
---
drivers/net/ethernet/intel/ice/ice.h | 1 +
drivers/net/ethernet/intel/ice/ice_ethtool.c | 200 +--
drivers/net/ethernet/intel/ice/ice_main.c| 2 +
3 files changed, 138 insertions(+), 65 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice.h
b/drivers/net/ethernet/intel/ice/ice.h
index fcaa5c3b8ec0..988b177d9388 100644
--- a/drivers/net/ethernet/intel/ice/ice.h
+++ b/drivers/net/ethernet/intel/ice/ice.h
@@ -960,6 +960,7 @@ int ice_stop(struct net_device *netdev);
void ice_service_task_schedule(struct ice_pf *pf);
int ice_load(struct ice_pf *pf);
void ice_unload(struct ice_pf *pf);
+void ice_adv_lnk_speed_maps_init(void);
/**
* ice_set_rdma_cap - enable RDMA support
diff --git a/drivers/net/ethernet/intel/ice/ice_ethtool.c
b/drivers/net/ethernet/intel/ice/ice_ethtool.c
index d3cb08e66dcb..b027788c42f6 100644
--- a/drivers/net/ethernet/intel/ice/ice_ethtool.c
+++ b/drivers/net/ethernet/intel/ice/ice_ethtool.c
@@ -345,6 +345,86 @@ static const struct ice_priv_flag
ice_gstrings_priv_flags[] = {
#define ICE_PRIV_FLAG_ARRAY_SIZE ARRAY_SIZE(ice_gstrings_priv_flags)
+static const u32 ice_adv_lnk_speed_100[] __initconst = {
+ ETHTOOL_LINK_MODE_100baseT_Full_BIT,
+};
+
+static const u32 ice_adv_lnk_speed_1000[] __initconst = {
+ ETHTOOL_LINK_MODE_1000baseX_Full_BIT,
+ ETHTOOL_LINK_MODE_1000baseT_Full_BIT,
+ ETHTOOL_LINK_MODE_1000baseKX_Full_BIT,
+};
+
+static const u32 ice_adv_lnk_speed_2500[] __initconst = {
+ ETHTOOL_LINK_MODE_2500baseT_Full_BIT,
+ ETHTOOL_LINK_MODE_2500baseX_Full_BIT,
+};
+
+static const u32 ice_adv_lnk_speed_5000[] __initconst = {
+ ETHTOOL_LINK_MODE_5000baseT_Full_BIT,
+};
+
+static const u32 ice_adv_lnk_speed_1[] __initconst = {
+ ETHTOOL_LINK_MODE_1baseT_Full_BIT,
+ ETHTOOL_LINK_MODE_1baseKR_Full_BIT,
+ ETHTOOL_LINK_MODE_1baseSR_Full_BIT,
+ ETHTOOL_LINK_MODE_1baseLR_Full_BIT,
+};
+
+static const u32 ice_adv_lnk_speed_25000[] __initconst = {
+ ETHTOOL_LINK_MODE_25000baseCR_Full_BIT,
+ ETHTOOL_LINK_MODE_25000baseSR_Full_BIT,
+ ETHTOOL_LINK_MODE_25000baseKR_Full_BIT,
+};
+
+static const u32 ice_adv_lnk_speed_4[] __initconst = {
+ ETHTOOL_LINK_MODE_4baseCR4_Full_BIT,
+ ETHTOOL_LINK_MODE_4baseSR4_Full_BIT,
+ ETHTOOL_LINK_MODE_4baseLR4_Full_BIT,
+ ETHTOOL_LINK_MODE_4baseKR4_Full_BIT,
+};
+
+static const u32 ice_adv_lnk_speed_5[] __initconst = {
+ ETHTOOL_LINK_MODE_5baseCR2_Full_BIT,
+ ETHTOOL_LINK_MODE_5baseKR2_Full_BIT,
+ ETHTOOL_LINK_MODE_5baseSR2_Full_BIT,
+};
+
+static const u32 ice_adv_lnk_speed_10[] __initconst = {
+ ETHTOOL_LINK_MODE_10baseCR4_Full_BIT,
+ ETHTOOL_LINK_MODE_10baseSR4_Full_BIT,
+ ETHTOOL_LINK_MODE_10baseLR4_ER4_Full_BIT,
+ ETHTOOL_LINK_MODE_10baseKR4_Full_BIT,
+ ETHTOOL_LINK_MODE_10baseCR2_Full_BIT,
+ ETHTOOL_LINK_MODE_10baseSR2_Full_BIT,
+ ETHTOOL_LINK_MODE_10baseKR2_Full_BIT,
+};
+
+#define ICE_ADV_LNK_SPEED_MAP(value) \
+{ \
+ .speed = SPEED_##value,\
+ .cap_arr= ice_adv_lnk_speed_##value,\
+ .arr_size = ARRAY_SIZE(ice_adv_lnk_speed_##value),\
+}
+
+static struct ethtool_forced_speed_map ice_adv_lnk_speed_maps[]
__ro_after_init = {
+ ICE_ADV_LNK_SPEED_MAP(100),
+ ICE_ADV_LNK_SPEED_MAP(1000),
+ ICE_ADV_LNK_SPEED_MAP(2500),
+ ICE_ADV_LNK_SPEED_MAP(5000),
+ ICE_ADV_LNK_SPEED_MAP(1),
+ ICE_ADV_LNK_SPEED_MAP(25000),
+ ICE_ADV_LNK_SPEED_MAP(4),
+ ICE_ADV_LNK_SPEED_MAP(5),
+ ICE_ADV_LNK_SPEED_MAP(10),
+};
+
+void __init ice_adv_lnk_speed_maps_init(void)
+{
+ ethtool_forced_speed_maps_init(ice_adv_lnk_speed_maps,
+ ARRAY_SIZE(ice_adv_lnk_speed_maps));
+}
+
static void
__ice_get_drvinfo(struct net_device *netdev, struct ethtool_drvinfo *drvinfo,
struct ice_vsi *vsi)
@@ -2007,6 +2087,55 @@ ice_get_link_ksettings(struct net_device *netdev,
return err;
}
+/**
+ * ice_speed_to_aq_link - Get AQ link speed by Ethtool forced speed
+ * @speed: ethtool forced speed
+ */
+static u16 ice_speed_to_aq_link(int speed)
+{
+ int aq_speed;
+
+ switch (speed) {
+ case SPEED_10:
+ aq_speed = ICE_AQ_LINK_SPEED_10MB;
+ break;
+ case SPEED_100:
+ aq_speed = ICE_AQ_LINK_SPEED_100MB;
+ break;
+ case SPEED_1000:
+ aq_sp
Re: [Intel-wired-lan] [PATCH net-next 2/4] dpll: spec: add support for pin-dpll signal phase offset/adjust
Wed, Sep 27, 2023 at 11:24:33AM CEST, arkadiusz.kubalew...@intel.com wrote: >Add new pin's attributes to dpll netlink spec: >- phase-offset - measured difference between phase of signals on pin > and dpll >- phase-adjust - adjustable value of pin's signal phase >- phase-adjust-min / phase-adjust-max - values for determining limits > for phase-adjust > >Signed-off-by: Arkadiusz Kubalewski >--- > Documentation/netlink/specs/dpll.yaml | 33 ++- > drivers/dpll/dpll_nl.c| 8 --- > drivers/dpll/dpll_nl.h| 2 +- > include/uapi/linux/dpll.h | 8 ++- > 4 files changed, 45 insertions(+), 6 deletions(-) > >diff --git a/Documentation/netlink/specs/dpll.yaml >b/Documentation/netlink/specs/dpll.yaml >index 8b86b28b47a6..dc057494101f 100644 >--- a/Documentation/netlink/specs/dpll.yaml >+++ b/Documentation/netlink/specs/dpll.yaml >@@ -1,7 +1,7 @@ > # SPDX-License-Identifier: ((GPL-2.0 WITH Linux-syscall-note) OR BSD-3-Clause) > > name: dpll >- >+version: 2 How is this supposed to work. You bump the version with every uapi extension? First time I see this. [...] >diff --git a/include/uapi/linux/dpll.h b/include/uapi/linux/dpll.h >index 20ef0718f8dc..050f51b48ef8 100644 >--- a/include/uapi/linux/dpll.h >+++ b/include/uapi/linux/dpll.h >@@ -7,7 +7,7 @@ > #define _UAPI_LINUX_DPLL_H > > #define DPLL_FAMILY_NAME "dpll" >-#define DPLL_FAMILY_VERSION 1 >+#define DPLL_FAMILY_VERSION 2 ___ Intel-wired-lan mailing list Intel-wired-lan@osuosl.org https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Re: [Intel-wired-lan] [PATCH net-next 1/4] dpll: docs: add support for pin signal phase offset/adjust
Wed, Sep 27, 2023 at 11:24:32AM CEST, arkadiusz.kubalew...@intel.com wrote: >Add dpll documentation on new pin's attributes: >- phase-offset - measured difference between phase of signals on pin > and dpll >- phase-adjust - adjustable value of pin's signal phase >- phase-adjust-min / phase-adjust-max - values for determining limits > for phase-adjust > >Signed-off-by: Arkadiusz Kubalewski >--- > Documentation/driver-api/dpll.rst | 53 ++- > 1 file changed, 52 insertions(+), 1 deletion(-) > >diff --git a/Documentation/driver-api/dpll.rst >b/Documentation/driver-api/dpll.rst >index bb52f1b8c0be..59634a3513bd 100644 >--- a/Documentation/driver-api/dpll.rst >+++ b/Documentation/driver-api/dpll.rst >@@ -173,6 +173,47 @@ in order to configure active input of a MUX-type pin, the >user needs to > request desired pin state of the child pin on the parent pin, > as described in the ``MUX-type pins`` chapter. > >+Phase offset measurement and adjustment >+ >+ >+Device may provide ability to measure a phase difference between signals >+on a pin and its parent dpll device. If pin-dpll phase offset measurement >+is supported, it shall be provided with ``DPLL_A_PIN_PHASE_OFFSET`` >+attribute for each parent dpll device. >+ >+Device may also provide ability to adjust a signal phase on a pin. >+If pin phase adjustment is supported, minimal and maximal values that pin >+handle shall be provide to the user on ``DPLL_CMD_PIN_GET`` respond >+with ``DPLL_A_PIN_PHASE_ADJUST_MIN`` and ``DPLL_A_PIN_PHASE_ADJUST_MAX`` >+attributes. Configured phase adjust value is provided with >+``DPLL_A_PIN_PHASE_ADJUST`` attribute of a pin, and value change can be >+requested with the same attribute with ``DPLL_CMD_PIN_SET`` command. >+ >+ === == >+ ``DPLL_A_PIN_ID`` configured pin id >+ ``DPLL_A_PIN_PHASE_ADJUST_MIN`` attr minimum value of phase adjustment >+ ``DPLL_A_PIN_PHASE_ADJUST_MAX`` attr maximum value of phase adjustment >+ ``DPLL_A_PIN_PHASE_ADJUST`` attr configured value of phase >+ adjustment on parent dpll device >+ ``DPLL_A_PIN_PARENT_DEVICE``nested attribute for requesting >+ configuration on given parent dpll >+ device >+``DPLL_A_PIN_PARENT_ID`` parent dpll device id >+``DPLL_A_PIN_PHASE_OFFSET`` attr measured phase difference >+ between a pin and parent dpll device >+ === == >+ >+All phase related values are provided in pico seconds, which represents >+time differnece between signals phase. The negative value means that >+phase of signal on pin is earlier in time than dpll's signal. Positive >+value means that phase of signal on pin is later in time than signal of >+a dpll. >+ >+Phase adjust (also min and max) values are integers, but measured phase >+offset values are fractional with 3-digit decimal places and shell be >+divided with ``DPLL_PIN_PHASE_OFFSET_DIVIDER`` to get integer part and >+modulo divided to get fractional part. >+ > Configuration commands group > > >@@ -263,6 +304,12 @@ according to attribute purpose. >frequencies > ``DPLL_A_PIN_ANY_FREQUENCY_MIN`` attr minimum value of frequency > ``DPLL_A_PIN_ANY_FREQUENCY_MAX`` attr maximum value of frequency >+``DPLL_A_PIN_PHASE_ADJUST_MIN``attr minimum value of phase >+ adjustment >+``DPLL_A_PIN_PHASE_ADJUST_MAX``attr maximum value of phase >+ adjustment >+``DPLL_A_PIN_PHASE_ADJUST``attr configured value of phase >+ adjustment on parent device > ``DPLL_A_PIN_PARENT_DEVICE`` nested attr for each parent device >the pin is connected with > ``DPLL_A_PIN_PARENT_ID`` attr parent dpll device id >@@ -270,8 +317,10 @@ according to attribute purpose. >dpll device > ``DPLL_A_PIN_STATE`` attr state of pin on the parent >dpll device >- ``DPLL_A_PIN_DIRECTION`` attr direction of a pin on the >+ ``DPLL_A_PIN_DIRECTION`` attr direction of a pin on the Could be in a separate patch, it's not related to this one. >parent dpll device >+ ``DPLL_A_PIN_PHASE_OFFSET`` attr measured phase difference >+ between a pin and parent dpll > ``DPLL_A_PIN_PARENT_PIN`` nested attr for each parent pin >the pin is connected with > ``DPLL_A_PIN_PARENT_ID`` attr parent pin id >@@ -
Re: [Intel-wired-lan] [PATCH net-next 3/4] dpll: netlink/core: add support for pin-dpll signal phase offset/adjust
Mon, Oct 02, 2023 at 04:32:30PM CEST, arkadiusz.kubalew...@intel.com wrote:
>>From: Vadim Fedorenko
>>Sent: Wednesday, September 27, 2023 8:09 PM
>>
>>On 27/09/2023 10:24, Arkadiusz Kubalewski wrote:
>>> Add callback op (get) for pin-dpll phase-offset measurment.
>>> Add callback ops (get/set) for pin signal phase adjustment.
>>> Add min and max phase adjustment values to pin proprties.
>>> Invoke get callbacks when filling up the pin details to provide user
>>> with phase related attribute values.
>>> Invoke phase-adjust set callback when phase-adjust value is provided for
>>> pin-set request.
>>>
>>> Signed-off-by: Arkadiusz Kubalewski
>>
>>[...]
>>
>>> +static int
>>> +dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr
>>> *phase_adj_attr,
>>> + struct netlink_ext_ack *extack)
>>> +{
>>> + struct dpll_pin_ref *ref;
>>> + unsigned long i;
>>> + s32 phase_adj;
>>> + int ret;
>>> +
>>> + phase_adj = nla_get_s32(phase_adj_attr);
>>> + if (phase_adj > pin->prop->phase_range.max ||
>>> + phase_adj < pin->prop->phase_range.min) {
>>> + NL_SET_ERR_MSG(extack, "phase adjust value not supported");
>>> + return -EINVAL;
>>> + }
>>> + xa_for_each(&pin->dpll_refs, i, ref) {
>>> + const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
>>> + struct dpll_device *dpll = ref->dpll;
>>> +
>>> + if (!ops->phase_adjust_set)
>>> + return -EOPNOTSUPP;
>>
>>I'm thinking about this part. We can potentially have dpll devices with
>>different expectations on phase adjustments, right? And if one of them
>>won't be able to adjust phase (or will fail in the next line), then
>>netlink will return EOPNOTSUPP while _some_ of the devices will be
>>adjusted. Doesn't look great. Can we think about different way to apply
>>the change?
>>
>
>Well makes sense to me.
>
>Does following makes sense as a fix?
>We would call op for all devices which has been provided with the op.
>If device has no op -> add extack error, continue
Is it real to expect some of the device support this and others don't?
Is it true for ice?
If not, I would got for all-or-nothing here.
>If device fails to set -> add extack error, continue
>Function always returns 0.
>
>Thank you!
>Arkadiusz
>
>>
>>> + ret = ops->phase_adjust_set(pin,
>>> + dpll_pin_on_dpll_priv(dpll, pin),
>>> + dpll, dpll_priv(dpll), phase_adj,
>>> + extack);
>>> + if (ret)
>>> + return ret;
>>> + }
>>> + __dpll_pin_change_ntf(pin);
>>> +
>>> + return 0;
>>> +}
>>> +
___
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Re: [Intel-wired-lan] [PATCH net-next 3/4] dpll: netlink/core: add support for pin-dpll signal phase offset/adjust
On 02/10/2023 16:04, Jiri Pirko wrote:
Mon, Oct 02, 2023 at 04:32:30PM CEST, arkadiusz.kubalew...@intel.com wrote:
From: Vadim Fedorenko
Sent: Wednesday, September 27, 2023 8:09 PM
On 27/09/2023 10:24, Arkadiusz Kubalewski wrote:
Add callback op (get) for pin-dpll phase-offset measurment.
Add callback ops (get/set) for pin signal phase adjustment.
Add min and max phase adjustment values to pin proprties.
Invoke get callbacks when filling up the pin details to provide user
with phase related attribute values.
Invoke phase-adjust set callback when phase-adjust value is provided for
pin-set request.
Signed-off-by: Arkadiusz Kubalewski
[...]
+static int
+dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr
*phase_adj_attr,
+ struct netlink_ext_ack *extack)
+{
+ struct dpll_pin_ref *ref;
+ unsigned long i;
+ s32 phase_adj;
+ int ret;
+
+ phase_adj = nla_get_s32(phase_adj_attr);
+ if (phase_adj > pin->prop->phase_range.max ||
+ phase_adj < pin->prop->phase_range.min) {
+ NL_SET_ERR_MSG(extack, "phase adjust value not supported");
+ return -EINVAL;
+ }
+ xa_for_each(&pin->dpll_refs, i, ref) {
+ const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
+ struct dpll_device *dpll = ref->dpll;
+
+ if (!ops->phase_adjust_set)
+ return -EOPNOTSUPP;
I'm thinking about this part. We can potentially have dpll devices with
different expectations on phase adjustments, right? And if one of them
won't be able to adjust phase (or will fail in the next line), then
netlink will return EOPNOTSUPP while _some_ of the devices will be
adjusted. Doesn't look great. Can we think about different way to apply
the change?
Well makes sense to me.
Does following makes sense as a fix?
We would call op for all devices which has been provided with the op.
If device has no op -> add extack error, continue
Is it real to expect some of the device support this and others don't?
Is it true for ice?
If not, I would got for all-or-nothing here.
But nothing blocks vendors to provide such configuration. Should we
rollback the configuration? Otherwise we can easily make it
inconsistent.
I'm more thinking of checking if all the devices returned error (or
absence of operation callback) and then return error instead of 0 with
extack filled in.
If device fails to set -> add extack error, continue
Function always returns 0.
Thank you!
Arkadiusz
+ ret = ops->phase_adjust_set(pin,
+ dpll_pin_on_dpll_priv(dpll, pin),
+ dpll, dpll_priv(dpll), phase_adj,
+ extack);
+ if (ret)
+ return ret;
+ }
+ __dpll_pin_change_ntf(pin);
+
+ return 0;
+}
+
___
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Re: [Intel-wired-lan] [PATCH iwl-net v1] ice: reset first in crash dump kernels
On 9/19/2023 10:18 PM, Paul Menzel wrote:
> Dear Jesse,
>
>
> Thank you for your patch.
>
> Am 19.09.23 um 23:29 schrieb Jesse Brandeburg:
>> When booting into the crash dump kernels there are cases where upon
>> enabling the device, the system under test will panic or machine check.
>>
>> One such test is to
>> - load ice driver
>> $ modprobe ice
>> - enable SR-IOV (2 VFs)
>> $ echo 2 > /sys/class/net/eth0/device/sriov_num_vfs
>> - crash
>> echo c > /proc/sysrq-trigger
>
> Above you prepended a $.
Fixed in v2.
>
>> - load ice driver (or happens automatically)
>> modprobe ice
>> - crash during pcim_enable_device()
>>
>> Avoid this problem by issuing a FLR to the device via PCIe config space
>> on the crash kernel, to clear out any outstanding transactions and stop
>> all queues and interrupts. Restore config space afterword because the
>
> afterw*a*rd
Fixed in v2.
>
>> driver won't load successfully otherwise.
>
> Excuse my ignorance, could you please add, what the crashdump kernel
> does differently from the “normal” kernel, so this special handling is
> needed?
I added more description in the v2 commit message, I hope that helps.
In summary: the crashdump kernel is starting up on "dirty" state of
hardware, due to the surprise crash of the previously running kernel
that had running devices when it "panicked"
>
>> Reviewed-by: Przemek Kitszel
>> Signed-off-by: Jesse Brandeburg
>> ---
>> drivers/net/ethernet/intel/ice/ice_main.c | 15 +++
>> 1 file changed, 15 insertions(+)
>>
>> diff --git a/drivers/net/ethernet/intel/ice/ice_main.c
>> b/drivers/net/ethernet/intel/ice/ice_main.c
>> index c8286adae946..6550c46e4e36 100644
>> --- a/drivers/net/ethernet/intel/ice/ice_main.c
>> +++ b/drivers/net/ethernet/intel/ice/ice_main.c
>> @@ -6,6 +6,7 @@
>> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>> #include
>> +#include
>> #include "ice.h"
>> #include "ice_base.h"
>> #include "ice_lib.h"
>> @@ -5014,6 +5015,20 @@ ice_probe(struct pci_dev *pdev, const struct
>> pci_device_id __always_unused *ent)
>> return -EINVAL;
>> }
>> + /* when under a kdump kernel initiate a reset before enabling the
>> + * device in order to clear out any pending DMA transactions. These
>> + * transactions can cause some systems to machine check when doing
>> + * the pcim_enable_device() below.
>> + */
>> + if (is_kdump_kernel()) {
>> + pci_save_state(pdev);
>> + pci_clear_master(pdev);
>> + err = pcie_flr(pdev);
>> + if (err)
>> + return err;
>> + pci_restore_state(pdev);
>> + }
>> +
>
> Should this be added to the common PCI code? Maybe loop the PCI
> subsystem folks in?
Ok, I'll cc: linux-pci when I send v2.
>
>> /* this driver uses devres, see
>> * Documentation/driver-api/driver-model/devres.rst
>> */
>
>
> Kind regards,
>
> Paul
___
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
[Intel-wired-lan] [PATCH iwl-net v2] ice: reset first in crash dump kernels
When the system boots into the crash dump kernel after a panic, the ice
networking device may still have pending transactions that can cause errors
or machine checks when the device is re-enabled. This can prevent the crash
dump kernel from loading the driver or collecting the crash data.
To avoid this issue, perform a function level reset (FLR) on the ice device
via PCIe config space before enabling it on the crash kernel. This will
clear any outstanding transactions and stop all queues and interrupts.
Restore the config space after the FLR, otherwise it was found in testing
that the driver wouldn't load successfully.
The following sequence causes the original issue:
- Load the ice driver with modprobe ice
- Enable SR-IOV with 2 VFs: echo 2 > /sys/class/net/eth0/device/sriov_num_vfs
- Trigger a crash with echo c > /proc/sysrq-trigger
- Load the ice driver again (or let it load automatically) with modprobe ice
- The system crashes again during pcim_enable_device()
Reported-by: Vishal Agrawal
Reviewed-by: Przemek Kitszel
Signed-off-by: Jesse Brandeburg
---
v2: respond to list comments and update commit message
v1: initial version
---
drivers/net/ethernet/intel/ice/ice_main.c | 15 +++
1 file changed, 15 insertions(+)
diff --git a/drivers/net/ethernet/intel/ice/ice_main.c
b/drivers/net/ethernet/intel/ice/ice_main.c
index c8286adae946..6550c46e4e36 100644
--- a/drivers/net/ethernet/intel/ice/ice_main.c
+++ b/drivers/net/ethernet/intel/ice/ice_main.c
@@ -6,6 +6,7 @@
#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
#include
+#include
#include "ice.h"
#include "ice_base.h"
#include "ice_lib.h"
@@ -5014,6 +5015,20 @@ ice_probe(struct pci_dev *pdev, const struct
pci_device_id __always_unused *ent)
return -EINVAL;
}
+ /* when under a kdump kernel initiate a reset before enabling the
+* device in order to clear out any pending DMA transactions. These
+* transactions can cause some systems to machine check when doing
+* the pcim_enable_device() below.
+*/
+ if (is_kdump_kernel()) {
+ pci_save_state(pdev);
+ pci_clear_master(pdev);
+ err = pcie_flr(pdev);
+ if (err)
+ return err;
+ pci_restore_state(pdev);
+ }
+
/* this driver uses devres, see
* Documentation/driver-api/driver-model/devres.rst
*/
base-commit: 6a70e5cbedaf8ad10528ac9ac114f3ec20f422df
--
2.39.3
___
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Re: [Intel-wired-lan] [PATCH net-next 3/4] dpll: netlink/core: add support for pin-dpll signal phase offset/adjust
>From: Jiri Pirko
>Sent: Monday, October 2, 2023 5:04 PM
>
>Mon, Oct 02, 2023 at 04:32:30PM CEST, arkadiusz.kubalew...@intel.com wrote:
>>>From: Vadim Fedorenko
>>>Sent: Wednesday, September 27, 2023 8:09 PM
>>>
>>>On 27/09/2023 10:24, Arkadiusz Kubalewski wrote:
Add callback op (get) for pin-dpll phase-offset measurment.
Add callback ops (get/set) for pin signal phase adjustment.
Add min and max phase adjustment values to pin proprties.
Invoke get callbacks when filling up the pin details to provide user
with phase related attribute values.
Invoke phase-adjust set callback when phase-adjust value is provided
for
pin-set request.
Signed-off-by: Arkadiusz Kubalewski
>>>
>>>[...]
>>>
+static int
+dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr
*phase_adj_attr,
+ struct netlink_ext_ack *extack)
+{
+ struct dpll_pin_ref *ref;
+ unsigned long i;
+ s32 phase_adj;
+ int ret;
+
+ phase_adj = nla_get_s32(phase_adj_attr);
+ if (phase_adj > pin->prop->phase_range.max ||
+ phase_adj < pin->prop->phase_range.min) {
+ NL_SET_ERR_MSG(extack, "phase adjust value not supported");
+ return -EINVAL;
+ }
+ xa_for_each(&pin->dpll_refs, i, ref) {
+ const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
+ struct dpll_device *dpll = ref->dpll;
+
+ if (!ops->phase_adjust_set)
+ return -EOPNOTSUPP;
>>>
>>>I'm thinking about this part. We can potentially have dpll devices with
>>>different expectations on phase adjustments, right? And if one of them
>>>won't be able to adjust phase (or will fail in the next line), then
>>>netlink will return EOPNOTSUPP while _some_ of the devices will be
>>>adjusted. Doesn't look great. Can we think about different way to apply
>>>the change?
>>>
>>
>>Well makes sense to me.
>>
>>Does following makes sense as a fix?
>>We would call op for all devices which has been provided with the op.
>>If device has no op -> add extack error, continue
>
>Is it real to expect some of the device support this and others don't?
>Is it true for ice?
>If not, I would got for all-or-nothing here.
>
Let's step back a bit.
The op itself is introduced as per pin-dpll tuple.. did this intentionally,
to inform each dpll that the offset has been changed - in case dplls are
controlled by separated driver/firmware instances but still sharing the pin.
Same way a pin frequency is being set, from user perspective on a pin, but
callback is called for each dpll the pin was registered with.
Whatever we do here, it shall be probably done for frequency_set() callback as
well.
The answers:
So far I don't know the device that might do it this way, it rather supports
phase_adjust or not. In theory we allow such behavior to be implemented, i.e.
pin is registered with 2 dplls, one has the callback, second not.
Current hardware of ice sets phase offset for a pin no matter on which dpll
device callback was invoked.
"all-or-nothing" - do you mean to check all callback returns and then decide
if it was successful?
Thank you!
Arkadiusz
>
>>If device fails to set -> add extack error, continue
>>Function always returns 0.
>>
>>Thank you!
>>Arkadiusz
>>
>>>
+ ret = ops->phase_adjust_set(pin,
+ dpll_pin_on_dpll_priv(dpll, pin),
+ dpll, dpll_priv(dpll), phase_adj,
+ extack);
+ if (ret)
+ return ret;
+ }
+ __dpll_pin_change_ntf(pin);
+
+ return 0;
+}
+
___
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Re: [Intel-wired-lan] [PATCH net-next 3/4] dpll: netlink/core: add support for pin-dpll signal phase offset/adjust
>From: Intel-wired-lan On Behalf Of
>Vadim Fedorenko
>Sent: Monday, October 2, 2023 5:09 PM
>
>On 02/10/2023 16:04, Jiri Pirko wrote:
>> Mon, Oct 02, 2023 at 04:32:30PM CEST, arkadiusz.kubalew...@intel.com
>> wrote:
From: Vadim Fedorenko
Sent: Wednesday, September 27, 2023 8:09 PM
On 27/09/2023 10:24, Arkadiusz Kubalewski wrote:
> Add callback op (get) for pin-dpll phase-offset measurment.
> Add callback ops (get/set) for pin signal phase adjustment.
> Add min and max phase adjustment values to pin proprties.
> Invoke get callbacks when filling up the pin details to provide user
> with phase related attribute values.
> Invoke phase-adjust set callback when phase-adjust value is provided
> for
> pin-set request.
>
> Signed-off-by: Arkadiusz Kubalewski
[...]
> +static int
> +dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr
> *phase_adj_attr,
> +struct netlink_ext_ack *extack)
> +{
> + struct dpll_pin_ref *ref;
> + unsigned long i;
> + s32 phase_adj;
> + int ret;
> +
> + phase_adj = nla_get_s32(phase_adj_attr);
> + if (phase_adj > pin->prop->phase_range.max ||
> + phase_adj < pin->prop->phase_range.min) {
> + NL_SET_ERR_MSG(extack, "phase adjust value not supported");
> + return -EINVAL;
> + }
> + xa_for_each(&pin->dpll_refs, i, ref) {
> + const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
> + struct dpll_device *dpll = ref->dpll;
> +
> + if (!ops->phase_adjust_set)
> + return -EOPNOTSUPP;
I'm thinking about this part. We can potentially have dpll devices with
different expectations on phase adjustments, right? And if one of them
won't be able to adjust phase (or will fail in the next line), then
netlink will return EOPNOTSUPP while _some_ of the devices will be
adjusted. Doesn't look great. Can we think about different way to apply
the change?
>>>
>>> Well makes sense to me.
>>>
>>> Does following makes sense as a fix?
>>> We would call op for all devices which has been provided with the op.
>>> If device has no op -> add extack error, continue
>>
>> Is it real to expect some of the device support this and others don't?
>> Is it true for ice?
>> If not, I would got for all-or-nothing here.
>>
>
>But nothing blocks vendors to provide such configuration. Should we
>rollback the configuration? Otherwise we can easily make it
>inconsistent.
Good point, in such case rollback might be required.
>
>I'm more thinking of checking if all the devices returned error (or
>absence of operation callback) and then return error instead of 0 with
>extack filled in.
>
Well, what if different devices would return different errors?
In general we would have to keep track of the error values returned in
such case.. Assuming one is different than the other - still need to error
extack them out? I guess it would be easier to return common error if there
were only failures and let the driver fill the errors on extack, smt like:
int miss_cb_num = 0, dev_num = 0, err_num;
xa_for_each(&pin->dpll_refs, i, ref) {
const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
struct dpll_device *dpll = ref->dpll;
dev_num++;
if (!ops->phase_adjust_set) {
miss_cb_num++;
continue;
}
ret = ops->phase_adjust_set(pin,
dpll_pin_on_dpll_priv(dpll, pin),
dpll, dpll_priv(dpll), phase_adj,
extack);
if (ret)
err_num++;
}
if (dev_num == miss_cb_num)
return -EOPNOTSUPP;
if (dev_num == err_num)
return -EINVAL;
__dpll_pin_change_ntf(pin);
return 0;
??
Thank you!
Arkadiusz
>>
>>> If device fails to set -> add extack error, continue
>>> Function always returns 0.
>>>
>>> Thank you!
>>> Arkadiusz
>>>
> + ret = ops->phase_adjust_set(pin,
> + dpll_pin_on_dpll_priv(dpll, pin),
> + dpll, dpll_priv(dpll), phase_adj,
> + extack);
> + if (ret)
> + return ret;
> + }
> + __dpll_pin_change_ntf(pin);
> +
> + return 0;
> +}
> +
>
>___
>Intel-wired-lan mailing list
>Intel-wired-lan@osuosl.org
>https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
___
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Re: [Intel-wired-lan] [PATCH iwl-net v2] ice: reset first in crash dump kernels
Jesse Brandeburg wrote:
>When the system boots into the crash dump kernel after a panic, the ice
>networking device may still have pending transactions that can cause errors
>or machine checks when the device is re-enabled. This can prevent the crash
>dump kernel from loading the driver or collecting the crash data.
>
>To avoid this issue, perform a function level reset (FLR) on the ice device
>via PCIe config space before enabling it on the crash kernel. This will
>clear any outstanding transactions and stop all queues and interrupts.
>Restore the config space after the FLR, otherwise it was found in testing
>that the driver wouldn't load successfully.
How does this differ from ading "reset_devices" to the crash
kernel command line, per Documentation/admin-guide/kdump/kdump.rst?
-J
>The following sequence causes the original issue:
>- Load the ice driver with modprobe ice
>- Enable SR-IOV with 2 VFs: echo 2 > /sys/class/net/eth0/device/sriov_num_vfs
>- Trigger a crash with echo c > /proc/sysrq-trigger
>- Load the ice driver again (or let it load automatically) with modprobe ice
>- The system crashes again during pcim_enable_device()
>
>Reported-by: Vishal Agrawal
>Reviewed-by: Przemek Kitszel
>Signed-off-by: Jesse Brandeburg
>---
>v2: respond to list comments and update commit message
>v1: initial version
>---
> drivers/net/ethernet/intel/ice/ice_main.c | 15 +++
> 1 file changed, 15 insertions(+)
>
>diff --git a/drivers/net/ethernet/intel/ice/ice_main.c
>b/drivers/net/ethernet/intel/ice/ice_main.c
>index c8286adae946..6550c46e4e36 100644
>--- a/drivers/net/ethernet/intel/ice/ice_main.c
>+++ b/drivers/net/ethernet/intel/ice/ice_main.c
>@@ -6,6 +6,7 @@
> #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
>
> #include
>+#include
> #include "ice.h"
> #include "ice_base.h"
> #include "ice_lib.h"
>@@ -5014,6 +5015,20 @@ ice_probe(struct pci_dev *pdev, const struct
>pci_device_id __always_unused *ent)
> return -EINVAL;
> }
>
>+ /* when under a kdump kernel initiate a reset before enabling the
>+ * device in order to clear out any pending DMA transactions. These
>+ * transactions can cause some systems to machine check when doing
>+ * the pcim_enable_device() below.
>+ */
>+ if (is_kdump_kernel()) {
>+ pci_save_state(pdev);
>+ pci_clear_master(pdev);
>+ err = pcie_flr(pdev);
>+ if (err)
>+ return err;
>+ pci_restore_state(pdev);
>+ }
>+
> /* this driver uses devres, see
>* Documentation/driver-api/driver-model/devres.rst
>*/
>
>base-commit: 6a70e5cbedaf8ad10528ac9ac114f3ec20f422df
>--
>2.39.3
>
---
-Jay Vosburgh, jay.vosbu...@canonical.com
___
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Re: [Intel-wired-lan] [PATCH iwl-next 3/4] ice: make ice_get_pf_c827_idx static
> -Original Message- > From: Intel-wired-lan On Behalf Of Jacob > Keller > Sent: Wednesday, September 20, 2023 5:05 AM > To: Intel Wired LAN > Cc: Keller, Jacob E ; Nguyen, Anthony L > > Subject: [Intel-wired-lan] [PATCH iwl-next 3/4] ice: make ice_get_pf_c827_idx > static > > The ice_get_pf_c827_idx function is only called inside of ice_ptp_hw.c, so > there is no reason to export it. Mark it static and remove the declaration > from ice_ptp_hw.h > > Signed-off-by: Jacob Keller > --- > drivers/net/ethernet/intel/ice/ice_ptp_hw.c | 2 +- > drivers/net/ethernet/intel/ice/ice_ptp_hw.h | 1 - > 2 files changed, 1 insertion(+), 2 deletions(-) > Tested-by: Pucha Himasekhar Reddy (A Contingent worker at Intel) ___ Intel-wired-lan mailing list Intel-wired-lan@osuosl.org https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
[Intel-wired-lan] [tnguy-net-queue:dev-queue] BUILD SUCCESS 9e8622c29420f8067533e6d2c83c898da168103d
tree/branch: https://git.kernel.org/pub/scm/linux/kernel/git/tnguy/net-queue.git dev-queue branch HEAD: 9e8622c29420f8067533e6d2c83c898da168103d ice: block default rule setting on LAG interface elapsed time: 737m configs tested: 107 configs skipped: 2 The following configs have been built successfully. More configs may be tested in the coming days. tested configs: alpha allnoconfig gcc alphaallyesconfig gcc alpha defconfig gcc arc allmodconfig gcc arc allnoconfig gcc arc allyesconfig gcc arc defconfig gcc arc randconfig-001-20231003 gcc arm allmodconfig gcc arm allnoconfig gcc arm allyesconfig gcc arm defconfig gcc arm randconfig-001-20231003 gcc arm64allmodconfig gcc arm64 allnoconfig gcc arm64allyesconfig gcc arm64 defconfig gcc csky allmodconfig gcc csky allnoconfig gcc csky allyesconfig gcc cskydefconfig gcc i386 allmodconfig gcc i386 allnoconfig gcc i386 allyesconfig gcc i386 buildonly-randconfig-001-20231003 gcc i386 buildonly-randconfig-002-20231003 gcc i386 buildonly-randconfig-003-20231003 gcc i386 buildonly-randconfig-004-20231003 gcc i386 buildonly-randconfig-005-20231003 gcc i386 buildonly-randconfig-006-20231003 gcc i386 debian-10.3 gcc i386defconfig gcc i386 randconfig-001-20231003 gcc i386 randconfig-002-20231003 gcc i386 randconfig-003-20231003 gcc i386 randconfig-004-20231003 gcc i386 randconfig-005-20231003 gcc i386 randconfig-006-20231003 gcc loongarchallmodconfig gcc loongarch allnoconfig gcc loongarchallyesconfig gcc loongarch defconfig gcc loongarch randconfig-001-20231003 gcc m68k allmodconfig gcc m68k allnoconfig gcc m68k allyesconfig gcc m68kdefconfig gcc microblaze allmodconfig gcc microblazeallnoconfig gcc microblaze allyesconfig gcc microblaze defconfig gcc mips allmodconfig gcc mips allnoconfig gcc mips allyesconfig gcc nios2allmodconfig gcc nios2 allnoconfig gcc nios2allyesconfig gcc nios2 defconfig gcc openrisc allmodconfig gcc openrisc allnoconfig gcc openrisc allyesconfig gcc openriscdefconfig gcc parisc allmodconfig gcc pariscallnoconfig gcc parisc allyesconfig gcc parisc defconfig gcc parisc64defconfig gcc powerpc allmodconfig gcc powerpc allnoconfig gcc powerpc allyesconfig gcc riscvallmodconfig gcc riscv allnoconfig gcc riscvallyesconfig gcc riscv defconfig gcc riscv rv32_defconfig gcc s390 allmodconfig gcc s390 allnoconfig gcc s390 allyesconfig gcc s390defconfig gcc sh allmodconfig gcc shallnoconfig gcc sh allyesconfig gcc sh defconfig gcc sparcallmodconfig gcc sparc allnoconfig gcc sparc
[Intel-wired-lan] [PATCH iwl-net v1] ice: fix over-shifted variable
Since the introduction of the ice driver the code has been
double-shifting the RSS enabling field, because the define already has
shifts in it and can't have the regular pattern of "a << shiftval &
mask" applied.
Most places in the code got it right, but one line was still wrong. Fix
this one location for easy backports to stable. An in-progress patch
fixes the defines to "standard" and will be applied as part of the
regular -next process sometime after this one.
Fixes: d76a60ba7afb ("ice: Add support for VLANs and offloads")
Reviewed-by: Przemek Kitszel
CC: sta...@vger.kernel.org
Signed-off-by: Jesse Brandeburg
---
drivers/net/ethernet/intel/ice/ice_lib.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/intel/ice/ice_lib.c
b/drivers/net/ethernet/intel/ice/ice_lib.c
index 201570cd2e0b..0aac519cc298 100644
--- a/drivers/net/ethernet/intel/ice/ice_lib.c
+++ b/drivers/net/ethernet/intel/ice/ice_lib.c
@@ -1201,8 +1201,7 @@ static void ice_set_rss_vsi_ctx(struct ice_vsi_ctx *ctxt,
struct ice_vsi *vsi)
ctxt->info.q_opt_rss = ((lut_type << ICE_AQ_VSI_Q_OPT_RSS_LUT_S) &
ICE_AQ_VSI_Q_OPT_RSS_LUT_M) |
- ((hash_type << ICE_AQ_VSI_Q_OPT_RSS_HASH_S) &
-ICE_AQ_VSI_Q_OPT_RSS_HASH_M);
+ (hash_type & ICE_AQ_VSI_Q_OPT_RSS_HASH_M);
}
static void
base-commit: 6a70e5cbedaf8ad10528ac9ac114f3ec20f422df
--
2.39.3
___
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
[Intel-wired-lan] [PATCH iwl-net v1 1/1] igc: Fix ambiguity in the ethtool advertising
The 'ethtool_convert_link_mode_to_legacy_u32' method does not allow us to
advertise 2500M speed support and TP (twisted pair) properly. Convert to
'ethtool_link_ksettings_test_link_mode' to advertise supported speed and
eliminate ambiguity.
Fixes: 8c5ad0dae93c ("igc: Add ethtool support")
Suggested-by: Dima Ruinskiy
Suggested-by: Vitaly Lifshits
Signed-off-by: Sasha Neftin
---
drivers/net/ethernet/intel/igc/igc_ethtool.c | 35 ++--
1 file changed, 25 insertions(+), 10 deletions(-)
diff --git a/drivers/net/ethernet/intel/igc/igc_ethtool.c
b/drivers/net/ethernet/intel/igc/igc_ethtool.c
index 7ab6dd58e400..dd8a9d27a167 100644
--- a/drivers/net/ethernet/intel/igc/igc_ethtool.c
+++ b/drivers/net/ethernet/intel/igc/igc_ethtool.c
@@ -1817,7 +1817,7 @@ igc_ethtool_set_link_ksettings(struct net_device *netdev,
struct igc_adapter *adapter = netdev_priv(netdev);
struct net_device *dev = adapter->netdev;
struct igc_hw *hw = &adapter->hw;
- u32 advertising;
+ u16 advertised = 0;
/* When adapter in resetting mode, autoneg/speed/duplex
* cannot be changed
@@ -1842,18 +1842,33 @@ igc_ethtool_set_link_ksettings(struct net_device
*netdev,
while (test_and_set_bit(__IGC_RESETTING, &adapter->state))
usleep_range(1000, 2000);
- ethtool_convert_link_mode_to_legacy_u32(&advertising,
- cmd->link_modes.advertising);
- /* Converting to legacy u32 drops ETHTOOL_LINK_MODE_2500baseT_Full_BIT.
-* We have to check this and convert it to ADVERTISE_2500_FULL
-* (aka ETHTOOL_LINK_MODE_2500baseX_Full_BIT) explicitly.
-*/
- if (ethtool_link_ksettings_test_link_mode(cmd, advertising,
2500baseT_Full))
- advertising |= ADVERTISE_2500_FULL;
+ if (ethtool_link_ksettings_test_link_mode(cmd, advertising,
+ 2500baseT_Full))
+ advertised |= ADVERTISE_2500_FULL;
+
+ if (ethtool_link_ksettings_test_link_mode(cmd, advertising,
+ 1000baseT_Full))
+ advertised |= ADVERTISE_1000_FULL;
+
+ if (ethtool_link_ksettings_test_link_mode(cmd, advertising,
+ 100baseT_Full))
+ advertised |= ADVERTISE_100_FULL;
+
+ if (ethtool_link_ksettings_test_link_mode(cmd, advertising,
+ 100baseT_Half))
+ advertised |= ADVERTISE_100_HALF;
+
+ if (ethtool_link_ksettings_test_link_mode(cmd, advertising,
+ 10baseT_Full))
+ advertised |= ADVERTISE_10_FULL;
+
+ if (ethtool_link_ksettings_test_link_mode(cmd, advertising,
+ 10baseT_Half))
+ advertised |= ADVERTISE_10_HALF;
if (cmd->base.autoneg == AUTONEG_ENABLE) {
hw->mac.autoneg = 1;
- hw->phy.autoneg_advertised = advertising;
+ hw->phy.autoneg_advertised = advertised;
if (adapter->fc_autoneg)
hw->fc.requested_mode = igc_fc_default;
} else {
--
2.25.1
___
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Re: [Intel-wired-lan] [PATCH iwl-net v2] ice: reset first in crash dump kernels
On 10/2/2023 4:49 PM, Jay Vosburgh wrote: > Jesse Brandeburg wrote: > >> When the system boots into the crash dump kernel after a panic, the ice >> networking device may still have pending transactions that can cause errors >> or machine checks when the device is re-enabled. This can prevent the crash >> dump kernel from loading the driver or collecting the crash data. >> >> To avoid this issue, perform a function level reset (FLR) on the ice device >> via PCIe config space before enabling it on the crash kernel. This will >> clear any outstanding transactions and stop all queues and interrupts. >> Restore the config space after the FLR, otherwise it was found in testing >> that the driver wouldn't load successfully. > > How does this differ from ading "reset_devices" to the crash > kernel command line, per Documentation/admin-guide/kdump/kdump.rst? > > -J > Hi Jay, thanks for the question. That parameter is new to me, and upon looking into the parameter, it doesn't seem well documented. It also seems to only be used by storage controllers, and would basically result in the same code I already have. I suspect since it's a driver opt-in to the parameter, the difference would be 1) requiring the user to give the reset_devices parameter on the kdump kernel line (which is a big "if") and 2) less readable code than the current which does: if (is_kdump_kernel()) ... and the reset_devices way would be: if (reset_devices) ... There are several other examples in the networking tree using the method I ended up with in this change. I'd argue the preferred way in the networking tree is to use is_kdump_kernel(), which I like better because it doesn't require user input and shouldn't have any bad side effects from doing an extra reset in kdump. Also, this issue has already been tested to be fixed by this patch. I'd prefer to keep the patch as is, if that's ok with you. Thanks, Jesse ___ Intel-wired-lan mailing list Intel-wired-lan@osuosl.org https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
Re: [Intel-wired-lan] [PATCH net-next 3/4] dpll: netlink/core: add support for pin-dpll signal phase offset/adjust
Tue, Oct 03, 2023 at 01:10:39AM CEST, arkadiusz.kubalew...@intel.com wrote:
>>From: Intel-wired-lan On Behalf Of
>>Vadim Fedorenko
>>Sent: Monday, October 2, 2023 5:09 PM
>>
>>On 02/10/2023 16:04, Jiri Pirko wrote:
>>> Mon, Oct 02, 2023 at 04:32:30PM CEST, arkadiusz.kubalew...@intel.com
>>> wrote:
> From: Vadim Fedorenko
> Sent: Wednesday, September 27, 2023 8:09 PM
>
> On 27/09/2023 10:24, Arkadiusz Kubalewski wrote:
>> Add callback op (get) for pin-dpll phase-offset measurment.
>> Add callback ops (get/set) for pin signal phase adjustment.
>> Add min and max phase adjustment values to pin proprties.
>> Invoke get callbacks when filling up the pin details to provide user
>> with phase related attribute values.
>> Invoke phase-adjust set callback when phase-adjust value is provided
>> for
>> pin-set request.
>>
>> Signed-off-by: Arkadiusz Kubalewski
>
> [...]
>
>> +static int
>> +dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr
>> *phase_adj_attr,
>> + struct netlink_ext_ack *extack)
>> +{
>> +struct dpll_pin_ref *ref;
>> +unsigned long i;
>> +s32 phase_adj;
>> +int ret;
>> +
>> +phase_adj = nla_get_s32(phase_adj_attr);
>> +if (phase_adj > pin->prop->phase_range.max ||
>> +phase_adj < pin->prop->phase_range.min) {
>> +NL_SET_ERR_MSG(extack, "phase adjust value not
>> supported");
>> +return -EINVAL;
>> +}
>> +xa_for_each(&pin->dpll_refs, i, ref) {
>> +const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
>> +struct dpll_device *dpll = ref->dpll;
>> +
>> +if (!ops->phase_adjust_set)
>> +return -EOPNOTSUPP;
>
> I'm thinking about this part. We can potentially have dpll devices with
> different expectations on phase adjustments, right? And if one of them
> won't be able to adjust phase (or will fail in the next line), then
> netlink will return EOPNOTSUPP while _some_ of the devices will be
> adjusted. Doesn't look great. Can we think about different way to apply
> the change?
>
Well makes sense to me.
Does following makes sense as a fix?
We would call op for all devices which has been provided with the op.
If device has no op -> add extack error, continue
>>>
>>> Is it real to expect some of the device support this and others don't?
>>> Is it true for ice?
>>> If not, I would got for all-or-nothing here.
>>>
>>
>>But nothing blocks vendors to provide such configuration. Should we
>>rollback the configuration? Otherwise we can easily make it
>>inconsistent.
>
>Good point, in such case rollback might be required.
>
>>
>>I'm more thinking of checking if all the devices returned error (or
>>absence of operation callback) and then return error instead of 0 with
>>extack filled in.
>>
>
>Well, what if different devices would return different errors?
>In general we would have to keep track of the error values returned in
>such case.. Assuming one is different than the other - still need to error
>extack them out? I guess it would be easier to return common error if there
In this case, it is common to return the first error hit and bail out,
not trying the rest.
>were only failures and let the driver fill the errors on extack, smt like:
>
> int miss_cb_num = 0, dev_num = 0, err_num;
>
> xa_for_each(&pin->dpll_refs, i, ref) {
> const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
> struct dpll_device *dpll = ref->dpll;
>
> dev_num++;
> if (!ops->phase_adjust_set) {
> miss_cb_num++;
> continue;
> }
> ret = ops->phase_adjust_set(pin,
> dpll_pin_on_dpll_priv(dpll, pin),
> dpll, dpll_priv(dpll), phase_adj,
> extack);
> if (ret)
> err_num++;
> }
> if (dev_num == miss_cb_num)
> return -EOPNOTSUPP;
> if (dev_num == err_num)
> return -EINVAL;
> __dpll_pin_change_ntf(pin);
> return 0;
>
>??
>
>Thank you!
>Arkadiusz
>
>>>
If device fails to set -> add extack error, continue
Function always returns 0.
Thank you!
Arkadiusz
>
>> +ret = ops->phase_adjust_set(pin,
>> +dpll_pin_on_dpll_priv(dpll,
>> pin),
>> +dpll, dpll_priv(dpll),
>> phase_adj,
>> +extack);
>> +if (ret)
>> +return ret;
>> +}
>>
Re: [Intel-wired-lan] [PATCH net-next 3/4] dpll: netlink/core: add support for pin-dpll signal phase offset/adjust
Tue, Oct 03, 2023 at 01:03:00AM CEST, arkadiusz.kubalew...@intel.com wrote:
>>From: Jiri Pirko
>>Sent: Monday, October 2, 2023 5:04 PM
>>
>>Mon, Oct 02, 2023 at 04:32:30PM CEST, arkadiusz.kubalew...@intel.com wrote:
From: Vadim Fedorenko
Sent: Wednesday, September 27, 2023 8:09 PM
On 27/09/2023 10:24, Arkadiusz Kubalewski wrote:
> Add callback op (get) for pin-dpll phase-offset measurment.
> Add callback ops (get/set) for pin signal phase adjustment.
> Add min and max phase adjustment values to pin proprties.
> Invoke get callbacks when filling up the pin details to provide user
> with phase related attribute values.
> Invoke phase-adjust set callback when phase-adjust value is provided
> for
> pin-set request.
>
> Signed-off-by: Arkadiusz Kubalewski
[...]
> +static int
> +dpll_pin_phase_adj_set(struct dpll_pin *pin, struct nlattr
> *phase_adj_attr,
> +struct netlink_ext_ack *extack)
> +{
> + struct dpll_pin_ref *ref;
> + unsigned long i;
> + s32 phase_adj;
> + int ret;
> +
> + phase_adj = nla_get_s32(phase_adj_attr);
> + if (phase_adj > pin->prop->phase_range.max ||
> + phase_adj < pin->prop->phase_range.min) {
> + NL_SET_ERR_MSG(extack, "phase adjust value not supported");
> + return -EINVAL;
> + }
> + xa_for_each(&pin->dpll_refs, i, ref) {
> + const struct dpll_pin_ops *ops = dpll_pin_ops(ref);
> + struct dpll_device *dpll = ref->dpll;
> +
> + if (!ops->phase_adjust_set)
> + return -EOPNOTSUPP;
I'm thinking about this part. We can potentially have dpll devices with
different expectations on phase adjustments, right? And if one of them
won't be able to adjust phase (or will fail in the next line), then
netlink will return EOPNOTSUPP while _some_ of the devices will be
adjusted. Doesn't look great. Can we think about different way to apply
the change?
>>>
>>>Well makes sense to me.
>>>
>>>Does following makes sense as a fix?
>>>We would call op for all devices which has been provided with the op.
>>>If device has no op -> add extack error, continue
>>
>>Is it real to expect some of the device support this and others don't?
>>Is it true for ice?
>>If not, I would got for all-or-nothing here.
>>
>
>Let's step back a bit.
>The op itself is introduced as per pin-dpll tuple.. did this intentionally,
>to inform each dpll that the offset has been changed - in case dplls are
>controlled by separated driver/firmware instances but still sharing the pin.
>Same way a pin frequency is being set, from user perspective on a pin, but
>callback is called for each dpll the pin was registered with.
>Whatever we do here, it shall be probably done for frequency_set() callback as
>well.
>
>The answers:
>So far I don't know the device that might do it this way, it rather supports
>phase_adjust or not. In theory we allow such behavior to be implemented, i.e.
>pin is registered with 2 dplls, one has the callback, second not.
If there is only theoretical device like that now, implement
all-or-nothing. If such theoretical device appears in real, this could
be changed. The UAPI would not change, no problem.
>Current hardware of ice sets phase offset for a pin no matter on which dpll
>device callback was invoked.
>"all-or-nothing" - do you mean to check all callback returns and then decide
>if it was successful?
Check if all dplls have ops and only perform the action in such case. In
case one of the dplls does not have the op filled, return -EOPNOTSUPP.
Regarding the successful/failed op, I think you can just return. In
these cases, when user performs multiaction cmd, he should be prepared
to deal with consequences if part of this cmd fails. We don't have
rollback for any other multiaction cmd in dpll, I don't see why this
should be treated differently.
>
>Thank you!
>Arkadiusz
>
>>
>>>If device fails to set -> add extack error, continue
>>>Function always returns 0.
>>>
>>>Thank you!
>>>Arkadiusz
>>>
> + ret = ops->phase_adjust_set(pin,
> + dpll_pin_on_dpll_priv(dpll, pin),
> + dpll, dpll_priv(dpll), phase_adj,
> + extack);
> + if (ret)
> + return ret;
> + }
> + __dpll_pin_change_ntf(pin);
> +
> + return 0;
> +}
> +
>
___
Intel-wired-lan mailing list
Intel-wired-lan@osuosl.org
https://lists.osuosl.org/mailman/listinfo/intel-wired-lan
