Re: [PATCH net-next 3/6] net: lan966x: add support for interrupts from analyzer

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

Hi Jakub,

On Fri, 3 Dec 2021 11:46:42 +0100 Horatiu Vultur wrote:

quoted

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>

quoted

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

quoted

+     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;
+}

quoted

+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 =

quoted

+}
+
+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/

quoted

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

quoted

+                         mac_entry->port_index == dest_idx) {
+                             raw_entries[column].process = true;
+                             founded = true;
+                             break;
+                     }
+             }

-- 
/Horatiu