IronPort-SDR: +4OBppKR0MVRMNiqXr7QfLHFNcuaZ5Ut5RRC7yBandQmsoAxU1I3P1a9J1SJg3FdCzIxpCAjCl
 i/HjpEI4RtvamtrlbN/rI17tLvqetkTw1E0TqzBGNKPKMdT+4azGs7hgNYYkynfck8Wp50k/GV
 wWqPeoF8RPUgbyG1jlNnW5Xt7DoQPeTbmoHm0kkvqpc3YkiDYxQTf1DYiyQivnlQMHs3PWOMs5
 9OUy967MtoZezxx+flO3WfWuFRMakjodQJWb+Av/k5PBdVbREZ00SpCKjX3S12YT1yORM6ejyl
 5CqGx/viqkfRevfX6qRrZPjS
Date:   Tue, 7 Dec 2021 13:03:05 +0100
From:   Horatiu Vultur <horatiu.vultur@microchip.com>
To:     Jakub Kicinski <kuba@kernel.org>
CC:     <davem@davemloft.net>, <robh+dt@kernel.org>,
        <UNGLinuxDriver@microchip.com>, <linux@armlinux.org.uk>,
        <netdev@vger.kernel.org>, <devicetree@vger.kernel.org>,
        <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH net-next 3/6] net: lan966x: add support for interrupts
 from analyzer
Message-ID: <20211207120305.atm3xhyhz4xl7vqw@soft-dev3-1.localhost>
References: <20211203104645.1476704-1-horatiu.vultur@microchip.com>
 <20211203104645.1476704-4-horatiu.vultur@microchip.com>
 <20211206182456.4494c5f6@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
MIME-Version: 1.0
Content-Type: text/plain; charset="utf-8"
Content-Disposition: inline
In-Reply-To: <20211206182456.4494c5f6@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
Precedence: bulk

The 12/06/2021 18:24, Jakub Kicinski wrote:

Hi Jakub,

> 
> On Fri, 3 Dec 2021 11:46:42 +0100 Horatiu Vultur wrote:
> > This patch adds support for handling the interrupts generated by the
> > analyzer. Currently, only the MAC table generates these interrupts.
> > The MAC table will generate an interrupt whenever it learns or forgets
> > an entry in the table. It is the SW responsibility figure out which
> > entries were added/removed.
> >
> > Signed-off-by: Horatiu Vultur <horatiu.vultur@microchip.com>
> 
> > +static struct lan966x_mac_entry *lan966x_mac_alloc_entry(struct lan966x *lan966x,
> > +                                                      const unsigned char *mac,
> > +                                                      u16 vid, u16 port_index)
> > +{
> > +     struct lan966x_mac_entry *mac_entry;
> > +
> > +     mac_entry = devm_kzalloc(lan966x->dev,
> > +                              sizeof(*mac_entry), GFP_ATOMIC);
> 
> Is it really necessary to use devm_ allocation for the mac entries?
> It's 2x memory overhead.

It is not necessary.

> 
> Also why GFP_ATOMIC? Memory allocations are _a lot_ less likely with
> GFP_KERNEL.

Initially I thought this is called also in some context where it
couldn't sleep. But I was wrong.

I will change these in the next version.

> 
> > +     if (!mac_entry)
> > +             return NULL;
> > +
> > +     memcpy(mac_entry->mac, mac, ETH_ALEN);
> > +     mac_entry->vid = vid;
> > +     mac_entry->port_index = port_index;
> > +     mac_entry->row = LAN966X_MAC_INVALID_ROW;
> > +     return mac_entry;
> > +}
> 
> > +static void lan966x_mac_process_raw_entry(struct lan966x_mac_raw_entry *raw_entry,
> > +                                       u8 *mac, u16 *vid, u32 *dest_idx)
> > +{
> > +     mac[0] = (raw_entry->mach >> 8)  & 0xff;
> > +     mac[1] = (raw_entry->mach >> 0)  & 0xff;
> > +     mac[2] = (raw_entry->macl >> 24) & 0xff;
> > +     mac[3] = (raw_entry->macl >> 16) & 0xff;
> > +     mac[4] = (raw_entry->macl >> 8)  & 0xff;
> > +     mac[5] = (raw_entry->macl >> 0)  & 0xff;
> > +
> > +     *vid = (raw_entry->mach >> 16) & 0xfff;
> > +     *dest_idx  = ANA_MACACCESS_DEST_IDX_GET(raw_entry->maca);
> 
> Double space before =
> 
> > +}
> > +
> > +static void lan966x_mac_irq_process(struct lan966x *lan966x, u32 row,
> > +                                 struct lan966x_mac_raw_entry *raw_entries)
> > +{
> > +     struct lan966x_mac_entry *mac_entry, *tmp;
> > +     unsigned long flags;
> > +     char mac[ETH_ALEN];
> > +     u32 dest_idx;
> > +     u32 column;
> > +     u16 vid;
> > +
> > +     spin_lock_irqsave(&lan966x->mac_lock, flags);
> > +     list_for_each_entry_safe(mac_entry, tmp, &lan966x->mac_entries, list) {
> > +             bool founded = false;
> 
> s/founded/found/
> 
> > +             if (mac_entry->row != row)
> > +                     continue;
> > +
> > +             for (column = 0; column < LAN966X_MAC_COLUMNS; ++column) {
> > +                     /* All the valid entries are at the start of the row,
> > +                      * so when get one invalid entry it can just skip the
> > +                      * rest of the columns
> > +                      */
> > +                     if (!ANA_MACACCESS_VALID_GET(raw_entries[column].maca))
> > +                             break;
> > +
> > +                     lan966x_mac_process_raw_entry(&raw_entries[column],
> > +                                                   mac, &vid, &dest_idx);
> > +                     WARN_ON(dest_idx > lan966x->num_phys_ports);
> > +
> > +                     /* If the entry in SW is found, then there is nothing
> > +                      * to do
> > +                      */
> > +                     if (mac_entry->vid == vid &&
> > +                         ether_addr_equal(mac_entry->mac, mac) &&
> 
> You need to add __aligned(2) to mac, ether_addr_equal() needs aligned
> arguments.
> 
> > +                         mac_entry->port_index == dest_idx) {
> > +                             raw_entries[column].process = true;
> > +                             founded = true;
> > +                             break;
> > +                     }
> > +             }

-- 
/Horatiu