Date:   Tue, 7 Feb 2023 10:58:47 -0800
From:   Shannon Nelson <shannon.nelson@....com>
To:     Alexander Duyck <alexander.duyck@...il.com>
Cc:     netdev@...r.kernel.org, davem@...emloft.net, kuba@...nel.org,
        drivers@...sando.io
Subject: Re: [PATCH v2 net-next 3/3] ionic: add support for device Component
 Memory Buffers

On 2/7/23 10:45 AM, Alexander Duyck wrote:
> On Tue, Feb 7, 2023 at 10:24 AM Shannon Nelson <shannon.nelson@....com> wrote:
>>
>> On 2/6/23 1:36 PM, Alexander H Duyck wrote:
>>> On Mon, 2023-02-06 at 10:16 -0800, Shannon Nelson wrote:
>>>> The ionic device has on-board memory (CMB) that can be used
>>>> for descriptors as a way to speed descriptor access for faster
>>>> traffic processing.  It imposes a couple of restrictions so
>>>> is not on by default, but can be enabled through the ethtool
>>>> priv-flags.
>>>
>>> For the purposes of patch review it might be convinent to call out what
>>> those restrictions are as you enable the code below. I'm assuming it is
>>> mostly just the amount of space you can use, but if there is something
>>> else it would be useful to have that noted.
> 
> The big thing for me is to make sure you call out your limitations. I
> just want to make sure as the reviewer we know what to watch out for.
> My main concern is wanting to see it documented somewhere as I am
> assuming that it is mostly related to your MMIO size limitations.
> However I have concerns that there may be other items such as the use
> of write combining that would be nice to see called out somewhere as I
> assume that is only needed for performance and not some writeback
> limitation of the hardware.

Oh, ouch, in concentrating on the other comments, I missed responding to 
this one.

That "restrictions" note I think is over-stated here and left over from 
an earlier draft where queue/ring/mtu size changes were disallowed while 
CMB was running.  The main restriction now is just that the interface 
must be 'down' before enabling/disabing CMB.

I'll fix up the commit comment to be a little less alarming.

> 
>>>
>>>> @@ -390,6 +392,7 @@ static void ionic_remove(struct pci_dev *pdev)
>>>>
>>>>         ionic_port_reset(ionic);
>>>>         ionic_reset(ionic);
>>>> +     ionic_dev_teardown(ionic);
>>>>         pci_clear_master(pdev);
>>>>         ionic_unmap_bars(ionic);
>>>>         pci_release_regions(pdev);
>>>> diff --git a/drivers/net/ethernet/pensando/ionic/ionic_dev.c b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
>>>> index 626b9113e7c4..9b4bba2279ab 100644
>>>> --- a/drivers/net/ethernet/pensando/ionic/ionic_dev.c
>>>> +++ b/drivers/net/ethernet/pensando/ionic/ionic_dev.c
>>>> @@ -92,6 +92,7 @@ int ionic_dev_setup(struct ionic *ionic)
>>>>         unsigned int num_bars = ionic->num_bars;
>>>>         struct ionic_dev *idev = &ionic->idev;
>>>>         struct device *dev = ionic->dev;
>>>> +     int size;
>>>>         u32 sig;
>>>>
>>>>         /* BAR0: dev_cmd and interrupts */
>>>> @@ -133,9 +134,40 @@ int ionic_dev_setup(struct ionic *ionic)
>>>>         idev->db_pages = bar->vaddr;
>>>>         idev->phy_db_pages = bar->bus_addr;
>>>>
>>>> +     /* BAR2: optional controller memory mapping */
>>>> +     bar++;
>>>> +     mutex_init(&idev->cmb_inuse_lock);
>>>> +     if (num_bars < 3 || !ionic->bars[IONIC_PCI_BAR_CMB].len) {
>>>> +             idev->cmb_inuse = NULL;
>>>> +             idev->phy_cmb_pages = 0;
>>>> +             idev->cmb_npages = 0;
>>>> +             return 0;
>>>> +     }
>>>> +
>>>> +     idev->phy_cmb_pages = bar->bus_addr;
>>>> +     idev->cmb_npages = bar->len / PAGE_SIZE;
>>>> +     size = BITS_TO_LONGS(idev->cmb_npages) * sizeof(long);
>>>> +     idev->cmb_inuse = kzalloc(size, GFP_KERNEL);
>>>> +     if (!idev->cmb_inuse) {
>>>> +             idev->phy_cmb_pages = 0;
>>>> +             idev->cmb_npages = 0;
>>>> +     }
>>>> +
>>>
>>> Why not hold of on setting phy_cmb_pages and cmb_npages until after you
>>> have allocated the pages rather then resetting them in the event of
>>> failure?
>>
>> I need the values anyway to determine size, and the fail is unlikely, so
>> why bother with tmp variables in the middle?  Also, this clearly sets
>> idev->cmb_npages to 0 which is used as an indicator in the ethtool
>> handler that thm CMB pages feature is not available.
> 
> Everything seems to be based on cmb_inuse being set anyway. You could
> probably just leave the values set and not bother with zeroing them
> since cmb_inuse would be NULL in case of the failure.

I should be able to change ionic_cmb_rings_toggle() to look at cmb_inuse 
rather than cmb_npages.


> 
>>>
>>> Also is it really acceptable for this to fail silently?
>>
>> The fail would be from the memory allocation, which usually will already
>> have printed some message, and we are usually discouraged from adding
>> more "alloc failed" type messages.  Also, this is an optional feature,
>> not needed for normal driver operations, so we don't need to kill the
>> probe with an error code.  If the user tries to enable CMB, they will
>> get a message that it is not available.
> 
> The error I would be looking for would be specific to the feature
> failing to be enabled. The memory allocation would be harder to track
> down since you would have to pick it out of a backtrace. If we had a
> one line message about it failing to initialize that might be useful
> in debugging should an attempt to enable it fail.

Sure, I can add that.

Thanks,
sln

Powered by blists - more mailing lists