Tue, Apr 23, 2024 at 01:56:55PM CEST, sergey.temerkha...@intel.com wrote:
>
>
>> -----Original Message-----
>> From: Jiri Pirko <j...@resnulli.us>
>> Sent: Tuesday, April 23, 2024 1:36 PM
>> To: Temerkhanov, Sergey <sergey.temerkha...@intel.com>
>> Cc: intel-wired-...@lists.osuosl.org; net...@vger.kernel.org; Kitszel,
>> Przemyslaw <przemyslaw.kits...@intel.com>
>> Subject: Re: [PATCH iwl-next v2] ice: Extend auxbus device naming
>>
>> Tue, Apr 23, 2024 at 11:14:59AM CEST, sergey.temerkha...@intel.com
>> wrote:
>> >Include segment/domain number in the device name to distinguish
>> between
>> >PCI devices located on different root complexes in multi-segment
>> >configurations. Naming is changed from ptp_<bus>_<slot>_clk<clock> to
>> >ptp_<domain>_<bus>_<slot>_clk<clock>
>>
>> I don't understand why you need to encode pci properties of a parent device
>> into the auxiliary bus name. Could you please explain the motivation? Why
>> you need a bus instance per PF?
>>
>> The rest of the auxbus registrators don't do this. Could you please align?
>> Just
>> have one bus for ice driver and that's it.
>
>This patch adds support for multi-segment PCIe configurations.
>An auxdev is created for each adapter, which has a clock, in the system. There
>can be
You are trying to change auxiliary bus name.
>more than one adapter present, so there exists a possibility of device naming
>conflict.
>To avoid it, auxdevs are named according to the PCI geographical addresses of
>the adapters.
Why? It's the auxdev, the name should not contain anything related to
PCI, no reason for it. I asked for motivation, you didn't provide any.
Again, could you please avoid creating auxiliary bus per-PF and just
have one auxiliary but per-ice-driver?
>
>Some systems may have adapters connected to different RCs which represent
>separate
>PCI segments/domains. In such cases, BDF numbers for these adapters can
>match, triggering
>the naming conflict again. To avoid that, auxdev names are further extended to
>include the
>segment/domain number.
>
>>
>>
>> >
>> >v1->v2
>> >Rebase on top of the latest changes
>> >
>> >Signed-off-by: Sergey Temerkhanov <sergey.temerkha...@intel.com>
>> >Reviewed-by: Przemek Kitszel <przemyslaw.kits...@intel.com>
>> >---
>> > drivers/net/ethernet/intel/ice/ice_ptp.c | 18 ++++++++++++------
>> > 1 file changed, 12 insertions(+), 6 deletions(-)
>> >
>> >diff --git a/drivers/net/ethernet/intel/ice/ice_ptp.c
>> >b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> >index 402436b72322..744b102f7636 100644
>> >--- a/drivers/net/ethernet/intel/ice/ice_ptp.c
>> >+++ b/drivers/net/ethernet/intel/ice/ice_ptp.c
>> >@@ -2993,8 +2993,9 @@ ice_ptp_auxbus_create_id_table(struct ice_pf
>> *pf,
>> >const char *name) static int ice_ptp_register_auxbus_driver(struct
>> >ice_pf *pf) {
>> > struct auxiliary_driver *aux_driver;
>> >+ struct pci_dev *pdev = pf->pdev;
>> > struct ice_ptp *ptp;
>> >- char busdev[8] = {};
>> >+ char busdev[16] = {};
>> > struct device *dev;
>> > char *name;
>> > int err;
>> >@@ -3005,8 +3006,10 @@ static int ice_ptp_register_auxbus_driver(struct
>> ice_pf *pf)
>> > INIT_LIST_HEAD(&ptp->ports_owner.ports);
>> > mutex_init(&ptp->ports_owner.lock);
>> > if (ice_is_e810(&pf->hw))
>> >- sprintf(busdev, "%u_%u_", pf->pdev->bus->number,
>> >- PCI_SLOT(pf->pdev->devfn));
>> >+ snprintf(busdev, sizeof(busdev), "%u_%u_%u_",
>> >+ pci_domain_nr(pdev->bus),
>> >+ pdev->bus->number,
>> >+ PCI_SLOT(pdev->devfn));
>> > name = devm_kasprintf(dev, GFP_KERNEL, "ptp_%sclk%u", busdev,
>> > ice_get_ptp_src_clock_index(&pf->hw));
>> > if (!name)
>> >@@ -3210,8 +3213,9 @@ static void ice_ptp_release_auxbus_device(struct
>> >device *dev) static int ice_ptp_create_auxbus_device(struct ice_pf
>> >*pf) {
>> > struct auxiliary_device *aux_dev;
>> >+ struct pci_dev *pdev = pf->pdev;
>> > struct ice_ptp *ptp;
>> >- char busdev[8] = {};
>> >+ char busdev[16] = {};
>> > struct device *dev;
>> > char *name;
>> > int err;
>> >@@ -3224,8 +3228,10 @@ static int ice_ptp_create_auxbus_device(struct
>> ice_pf *pf)
>> > aux_dev = &ptp->port.aux_dev;
>> >
>> > if (ice_is_e810(&pf->hw))
>> >- sprintf(busdev, "%u_%u_", pf->pdev->bus->number,
>> >- PCI_SLOT(pf->pdev->devfn));
>> >+ snprintf(busdev, sizeof(busdev), "%u_%u_%u_",
>> >+ pci_domain_nr(pdev->bus),
>> >+ pdev->bus->number,
>> >+ PCI_SLOT(pdev->devfn));
>> >
>> > name = devm_kasprintf(dev, GFP_KERNEL, "ptp_%sclk%u", busdev,
>> > ice_get_ptp_src_clock_index(&pf->hw));
>> >--
>> >2.35.3
>> >
>> >
