Re: [RFC PATCH 2/2] net: selpcimac: Add driver for SEL PCIe network adapter
From: Christophe JAILLET
Date: Tue Oct 29 2024 - 13:21:20 EST
Le 28/10/2024 à 23:35, Robert Joslyn a écrit :
Add support for SEL FPGA based network adapters. The network device is
implemented as an FPGA IP core and enumerated by the selpvmf driver.
This is used on multiple devices, including:
- SEL-3350 mainboard
- SEL-3390E4 card
- SEL-3390T card
Signed-off-by: Robert Joslyn <robert_joslyn@xxxxxxxxxx>
---
Hi,
a few nitpicks below, should it help.
+/**
+ * selpcimac_rx_alloc_page() - Allocate a network fragment page.
+ */
+static struct page *selpcimac_rx_alloc_page(struct selpcimac_rx_ring *ring)
+{
+ struct page *page = dev_alloc_page();
+
+ return page;
return dev_alloc_page()?
+}
...
+/**
+ * selpcimac_cleanup_rxbd() - Remove an RXBD from hardware control.
+ *
+ * This is only safely called if RSTAT[RX_EN] == 0
+ *
+ * @desc: Pointer to the buffer descriptor to cleanup
+ */
+static void selpcimac_cleanup_rxbd(struct sel_rx_bd *desc)
+{
+ desc->rxbd_ctrl &= (cpu_to_le32(~(RXBD_CTRL_EMT)));
Un-needed outer ().
+}
+
+/**
+ * selpcimac_alloc_mapped_page() - Allocate a new page and map it for DMA.
+ *
+ * This function will ensure the passed in rx_buffer has a valid page to
+ * give to the hardware. A new page will be allocated only if the page
+ * pointer in the RX buffer is NULL. If the page pointer is not NULL, the
+ * page is already allocated and we are reusing it.
+ *
+ * @ring: Pointer to the ring that owns the RX buffer.
+ * @bi: Pointer to the RX buffer structure to allocate for.
+ *
+ * Returns: true if a page was successfully allocated, false otherwise.
+ */
+static bool selpcimac_alloc_mapped_page(struct selpcimac_rx_ring *ring,
+ struct selpcimac_rx_buffer *bi)
+{
+ struct page *page = bi->page;
+ dma_addr_t dma;
+
+ /* Already allocated, nothing to do */
+ if (page)
+ return true;
+
+ page = selpcimac_rx_alloc_page(ring);
+ if (likely(page)) {
+ dma = dma_map_page(ring->dev, page, 0,
+ PAGE_SIZE, DMA_FROM_DEVICE);
+ if (dma_mapping_error(ring->dev, dma)) {
+ selpcimac_rx_free_page(ring, page);
+ page = NULL;
+ } else {
+ bi->dma = dma;
+ bi->page = page;
+ bi->page_offset = 0;
+ }
+ }
+ return (page);
No need for ()
+}
...
+/**
+ * selpcimac_initialize_rx_ring() - Initialize the receive ring.
+ *
+ * This function allocates the buffer descriptor ring for the hardware,
+ * initializes it, and programs it to the hardware. It also starts the
+ * receive buffer allocation process.
+ *
+ * @ring: RX ring to initialize
+ *
+ * Returns: 0 if successful, -ENOMEM if we couldn't initialize the ring.
+ */
+static int selpcimac_initialize_rx_ring(struct selpcimac_rx_ring *ring)
+{
+ /* Allocation is an array of the currently configured number
+ * of RX buffer descriptors rounded up to the nearest 4k.
+ */
+ ring->size = (sizeof(struct sel_rx_bd) * ring->count);
+ ring->size = ALIGN(ring->size, 4096);
+
+ ring->desc = dma_alloc_coherent(ring->dev,
+ ring->size,
+ &ring->dma,
+ GFP_KERNEL);
+ if (!ring->desc)
+ goto err;
return directly and avoid the 'err' label?
+
+ WARN_ON_ONCE((ring->dma % SEL_DATA_ALIGN) != 0);
+
+ memset(ring->desc, 0, ring->size);
If I recollect correctly, dma_alloc_coherent() returns some zeroed memory.
+ ring->last_bd = ring->dma +
+ (ring->count - 1) * sizeof(struct sel_rx_bd);
+ ring->next_to_use = 0;
+ ring->next_to_clean = 0;
+
+ if (selpcimac_init_rx_buffers(ring))
+ goto err_free;
Why no simply propagate the error code from selpcimac_init_rx_buffers()?
Won't change anything, but more standard.
+
+ return 0;
+
+err_free:
+ dma_free_coherent(ring->dev, ring->size, ring->desc, ring->dma);
+ ring->desc = NULL;
+ ring->dma = 0;
+ ring->last_bd = 0;
+ ring->size = 0;
+err:
+ return -ENOMEM;
+}
...
+/**
+ * dump_sfp_id() - Print the id values for this ports SFP
+ *
+ * @mac: SEL MAC device
+ * @buff: Buffer to write id values to
+ * @size: The size of the buffer to write to
+ *
+ * @returns: Number of bytes written to buff
+ */
+static ssize_t dump_sfp_id(struct sel_pci_mac *mac, char *buff, int size)
+{
+ struct sel_sfp_id id;
+ ssize_t rc = 0;
+
+ if (sfp_read_id(mac, &id) < 0)
+ return 0;
+
+ if (id.sel_part[0] != 0) {
+ rc += snprintf(buff + rc, size - rc,
+ "SELPartNumber: %.4s-%.2s\n",
+ id.sel_part, id.sel_part_option);
I think that sysfs_emit_at() should be preferred in this function.
+ rc += snprintf(buff + rc, size - rc,
+ "SELSerialNumber: %.15s\n", id.sel_sn);
+ }
+ rc += snprintf(buff + rc, size - rc,
+ "Manufacturer: %.16s\n", id.name);
+ rc += snprintf(buff + rc, size - rc,
+ "PartNumber: %.16s\n", id.part);
+ rc += snprintf(buff + rc, size - rc,
+ "Version: %.4s\n", id.rev);
+ rc += snprintf(buff + rc, size - rc,
+ "SerialNumber: %.16s\n", id.sernum);
+ rc += snprintf(buff + rc, size - rc,
+ "DateCode: %.8s\n", id.datecode);
+ rc += snprintf(buff + rc, size - rc,
+ "Wavelength: %u nm\n", id.wavelength);
+ rc += snprintf(buff + rc, size - rc,
+ "LengthSingleMode: %u m\n", id.length_smf_km);
+ rc += snprintf(buff + rc, size - rc,
+ "Length50umOM2: %u m\n", id.length_om2);
+ rc += snprintf(buff + rc, size - rc,
+ "Length62p5umOM1: %u m\n", id.length_om1);
+ rc += snprintf(buff + rc, size - rc,
+ "LengthCopper: %u m\n", id.length_copper);
+ rc += snprintf(buff + rc, size - rc,
+ "Length50umOM3: %u m\n", id.length_om3);
+
+ return rc;
+}
+
+/**
+ * dump_sfp_diag() - Print the diag values for this ports SFP
+ *
+ * @mac: SEL MAC device
+ * @buff: Buffer to write diag values to
+ * @size: The size of the buffer to write to
+ *
+ * @returns: Number of bytes written to buff
+ */
+static ssize_t dump_sfp_diag(struct sel_pci_mac *mac, char *buff, int size)
+{
+ struct sel_sfp_diag diag;
+ ssize_t rc = 0;
+
+ if (sfp_read_diag(mac, &diag) < 0)
+ return 0;
+
+ rc += snprintf(buff + rc, size - rc,
+ "Temperature: %d C\n", diag.temp);
I think that sysfs_emit_at() should be preferred in this function.
+ rc += snprintf(buff + rc, size - rc,
+ "SupplyVoltage: %u uV\n", diag.vcc);
+ rc += snprintf(buff + rc, size - rc,
+ "TxBiasCurrent: %u uA\n", diag.tx_bias);
+ rc += snprintf(buff + rc, size - rc,
+ "TxPower: %u nW\n", diag.tx_power);
+ rc += snprintf(buff + rc, size - rc,
+ "RxPower: %u nW\n", diag.rx_power);
+
+ return rc;
+}
+
+/**
+ * __show_attr() - Return data for a specific attribute
+ *
+ * @dev: device object
+ * @attr: device attribute to show info for
+ * @buff: buffer to store output
+ * @attribute_type: the type of attribute to return information for
+ *
+ * Return: number of bytes stored in the buffer
+ */
+static ssize_t __show_attr(struct device *dev,
+ struct device_attribute *attr,
+ char *buff,
+ enum sel_dev_attributes attribute_type)
+{
+ struct selpcimac_platform_private *priv = dev_get_drvdata(dev->parent);
+ struct sel_pci_mac *mac = priv->priv;
+ ssize_t bytes_written = 0;
+ int length = 0;
+
+ switch (attribute_type) {
+ case SFP_CONFIGURATION:
+ bytes_written = sprintf(buff, "%d\n",
+ (u32)mac->sfp_configuration);
+ break;
+
+ case INTR_MOD_RATE:
+ bytes_written = sprintf(buff, "%d\n",
+ (u8)mac->interrupt_moderation_rate);
+ break;
+
+ case INTR_RX_ABS:
+ bytes_written = sprintf(buff, "%d\n",
+ ioread32(&mac->hw_mac->intr_moderation.rxabs));
+ break;
+
+ case INTR_RX_PACKET:
+ bytes_written = sprintf(buff, "%d\n",
+ ioread32(&mac->hw_mac->intr_moderation.rxpacket));
+ break;
+
+ case INTR_TX_ABS:
+ bytes_written = sprintf(buff, "%d\n",
+ ioread32(&mac->hw_mac->intr_moderation.txabs));
+ break;
+
+ case INTR_TX_PACKET:
+ bytes_written = sprintf(buff, "%d\n",
+ ioread32(&mac->hw_mac->intr_moderation.txpacket));
+ break;
+
+ case INTR_THROTTLE:
+ bytes_written = sprintf(buff, "%d\n",
+ ioread32(&mac->hw_mac->intr_moderation.throttle));
+ break;
+
+ case ETH_REGISTERED:
+ length += snprintf(buff + length,
You could remove 'lenght', as in all other cases.
I also think that sysfs_emit() should be preferred.
+ PAGE_SIZE - length,
+ "%s\n",
+ mac->netdev->name);
+ bytes_written = length;
+ break;
+
+ case SFP_ID:
+ bytes_written = dump_sfp_id(mac, buff, PAGE_SIZE);
+ break;
+
+ case SFP_DIAG:
+ bytes_written = dump_sfp_diag(mac, buff, PAGE_SIZE);
+ break;
+
+ default:
+ break;
+ }
+
+ return bytes_written;
+}
...
CJ