[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <ed18d03f-d467-5fe9-7b04-0fd474128d0b@pensando.io>
Date: Thu, 6 May 2021 20:26:05 -0700
From: Shannon Nelson <snelson@...sando.io>
To: Randy Dunlap <rdunlap@...radead.org>,
Jakub Kicinski <kuba@...nel.org>
Cc: netdev@...r.kernel.org, davem@...emloft.net, drivers@...sando.io,
Allen Hubbe <allenbh@...sando.io>
Subject: Re: [PATCH v3 net] ionic: fix ptp support config breakage
On 5/6/21 6:30 PM, Randy Dunlap wrote:
> On 5/6/21 6:16 PM, Randy Dunlap wrote:
>> On 5/6/21 6:12 PM, Randy Dunlap wrote:
>>> On 5/6/21 5:48 PM, Shannon Nelson wrote:
>>>> On 5/6/21 5:21 PM, Randy Dunlap wrote:
>>>>> On 5/6/21 5:15 PM, Jakub Kicinski wrote:
>>>>>> On Wed, 5 May 2021 21:18:46 -0700 Shannon Nelson wrote:
>>>>>>> Driver link failed with undefined references in some
>>>>>>> kernel config variations.
>>>>>> This is really vague and the patch is not very obvious.
> When IONIC=y and PTP_1588_CLOCK=m...
>
>>>>>>> ionic-y := ionic_main.o ionic_bus_pci.o ionic_devlink.o ionic_dev.o \
>>>>>>> ionic_debugfs.o ionic_lif.o ionic_rx_filter.o ionic_ethtool.o \
>>>>>>> - ionic_txrx.o ionic_stats.o ionic_fw.o
>>>>>>> -ionic-$(CONFIG_PTP_1588_CLOCK) += ionic_phc.o
>>>>>>> + ionic_txrx.o ionic_stats.o ionic_fw.o ionic_phc.o
>>>>>> So we'd replace a build dependency..
>>>>>>
>>>>>>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_phc.c b/drivers/net/ethernet/pensando/ionic/ionic_phc.c
>>>>>>> index a87c87e86aef..30c78808c45a 100644
>>>>>>> --- a/drivers/net/ethernet/pensando/ionic/ionic_phc.c
>>>>>>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_phc.c
>>>>>>> @@ -1,6 +1,8 @@
>>>>>>> // SPDX-License-Identifier: GPL-2.0
>>>>>>> /* Copyright(c) 2017 - 2021 Pensando Systems, Inc */
>>>>>>> +#if IS_ENABLED(CONFIG_PTP_1588_CLOCK)
>>>>>>> +
>>>>>>> #include <linux/netdevice.h>
>>>>>>> #include <linux/etherdevice.h>
>>>>>>> @@ -613,3 +615,4 @@ void ionic_lif_free_phc(struct ionic_lif *lif)
>>>>>>> devm_kfree(lif->ionic->dev, lif->phc);
>>>>>>> lif->phc = NULL;
>>>>>>> }
>>>>>>> +#endif /* IS_ENABLED(CONFIG_PTP_1588_CLOCK) */
>>>>>> .. with an ifdef around an entire file? Does not feel very clean.
>>>>>>
>>>>>> The construct of using:
>>>>>>
>>>>>> drv-$(CONFIG_PTP_1588_CLOCK) += ptp.o
>>>>>>
>>>>>> seems relatively common, why does it now work here?
>>>>>>
>>>>>> Maybe the config in question has PTP as a module and ionic built in?
>>>>>> Then you should add depends on PTP_1588_CLOCK || !PTP_1588_CLOCK.
>>>>>>
>>>>>> Maybe somehow the "ionic-y" confuses kbuild and it should be ionic-objs?
>>>>>>
>>>>>> At the very least we need a better explanation in the commit message.
>>>>>>
>>>>> I'll take a look if someone can point me to the .config file.
>>>>>
>>>> These are the notes I got from kernel test robot:
>>>> https://lore.kernel.org/lkml/202105041020.efEaBOYC-lkp@intel.com/
>>>> https://lore.kernel.org/lkml/202105041154.GrLZmjGh-lkp@intel.com/
>>>> https://lore.kernel.org/lkml/202105041634.paURyDp0-lkp@intel.com/
>>>> https://lore.kernel.org/lkml/202105050636.UXXDl7m2-lkp@intel.com/
>>> At first glance it looks like Jakub's suggestion of
>>>>>> Maybe the config in question has PTP as a module and ionic built in?
>>>>>> Then you should add depends on PTP_1588_CLOCK || !PTP_1588_CLOCK.
>>> is what is needed, but I'm still doing build testing ATM.
>> Nope, eat my words. These build issues are not about PTP.
>> I'm still looking.
> I have been trying to go to fast.. slow down, wait for the old computer.
>
> Back to Jakub's suggestion -- that works for me. (copy-paste, whitespace damaged)
>
>
> --- linux-next-20210506.orig/drivers/net/ethernet/pensando/Kconfig
> +++ linux-next-20210506/drivers/net/ethernet/pensando/Kconfig
> @@ -20,6 +20,7 @@ if NET_VENDOR_PENSANDO
> config IONIC
> tristate "Pensando Ethernet IONIC Support"
> depends on 64BIT && PCI
> + depends on PTP_1588_CLOCK || !PTP_1588_CLOCK
> select NET_DEVLINK
> select DIMLIB
> help
>
> If PTP_1588_CLOCK=m, the depends limits IONIC to =m (or disabled).
> If PTP_1588_CLOCK is disabled, IONIC can be any of y/m/n.
>
Thanks, I'll follow up with this.
sln
Powered by blists - more mailing lists