netdev - Re: [PATCH v2 net-next 3/3] ionic: add support for device Component Memory Buffers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Tue, 7 Feb 2023 10:45:51 -0800
From:   Alexander Duyck <alexander.duyck@...il.com>
To:     Shannon Nelson <shannon.nelson@....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 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.

> >
> >> @@ -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.

> >
> > 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.