Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1530652 > unrolled thread

Gigabit ethernet driver for Alacritechs SLIC devices (v3)

Back to article view | Back to linux.kernel

Contents

  Gigabit ethernet driver for Alacritechs SLIC devices (v3) Lino Sanfilippo <LinoSanfilippo@gmx.de> - 2016-11-26 13:30 +0100
    [PATCH v3 net-next 2/2] MAINTAINERS: add entry for slicoss ethernet driver Lino Sanfilippo <LinoSanfilippo@gmx.de> - 2016-11-26 13:30 +0100
    Re: Gigabit ethernet driver for Alacritechs SLIC devices (v3) Lino Sanfilippo <LinoSanfilippo@gmx.de> - 2016-11-26 13:40 +0100
    Re: [PATCH v3 net-next 1/2] net: ethernet: slicoss: add slicoss  gigabit ethernet driver Rami Rosen <roszenrami@gmail.com> - 2016-11-26 16:50 +0100
      Re: [PATCH v3 net-next 1/2] net: ethernet: slicoss: add slicoss  gigabit ethernet driver Lino Sanfilippo <LinoSanfilippo@gmx.de> - 2016-11-27 02:10 +0100
    Re: [PATCH v3 net-next 1/2] net: ethernet: slicoss: add slicoss  gigabit ethernet driver Markus Böhme <markus.boehme@mailbox.org> - 2016-11-27 19:10 +0100
      Re: [PATCH v3 net-next 1/2] net: ethernet: slicoss: add slicoss  gigabit ethernet driver Lino Sanfilippo <LinoSanfilippo@gmx.de> - 2016-11-28 21:50 +0100
        Re: [PATCH v3 net-next 1/2] net: ethernet: slicoss: add slicoss  gigabit ethernet driver Markus Böhme <markus.boehme@mailbox.org> - 2016-11-29 13:20 +0100
    Re: [PATCH v3 net-next 1/2] net: ethernet: slicoss: add slicoss  gigabit ethernet driver Florian Fainelli <f.fainelli@gmail.com> - 2016-11-28 06:00 +0100
      Re: [PATCH v3 net-next 1/2] net: ethernet: slicoss: add slicoss  gigabit ethernet driver Lino Sanfilippo <LinoSanfilippo@gmx.de> - 2016-11-28 22:50 +0100
        Re: [PATCH v3 net-next 1/2] net: ethernet: slicoss: add slicoss  gigabit ethernet driver Florian Fainelli <f.fainelli@gmail.com> - 2016-11-29 18:20 +0100
          Re: [PATCH v3 net-next 1/2] net: ethernet: slicoss: add slicoss  gigabit ethernet driver Lino Sanfilippo <LinoSanfilippo@gmx.de> - 2016-11-30 21:00 +0100

#1530652 — Gigabit ethernet driver for Alacritechs SLIC devices (v3)

Hi,

this is the third version of the slicoss gigabit ethernet driver (which is a
rework of the driver from Alacritech which can currently be found under
drivers/staging/slicoss). The driver is supposed to support Mojave, Oasis and
Kalahari cards, for both copper and fiber.

If this code is accepted the staging version can be removed.

The driver has been tested on a SEN2104ET adapter (4 Port PCIe copper).

v3:
- dont add defines to pci.h but instead put it into the drivers header file
(requested by Greg Kroah-Hartman)

v2:
- remove unusual padding in statistic strings (suggested by Andrew Lunn)
- for mdio register and bit names use defines from mii.h instead of own ones
  (suggested by Andrew Lunn)
- remove unused defines
- ensure PCI flush at two more places
- use mmiowb before lock to prevent mmio writes leaking out of lock
- fix some typos in comments
- add copyright and GPL header

Regards,
Lino 

[toc] | [next] | [standalone]

#1530653 — [PATCH v3 net-next 2/2] MAINTAINERS: add entry for slicoss ethernet driver

Add myself as maintainer for the slicoss ethernet driver.

Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
---
 MAINTAINERS | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/MAINTAINERS b/MAINTAINERS
index 6781a3f..bb9af28 100644
--- a/MAINTAINERS
+++ b/MAINTAINERS
@@ -562,6 +562,11 @@ T:	git git://linuxtv.org/anttip/media_tree.git
 S:	Maintained
 F:	drivers/media/usb/airspy/
 
+ALACRITECH GIGABIT ETHERNET DRIVER
+M:	Lino Sanfilippo <LinoSanfilippo@gmx.de>
+S:	Maintained
+F:	drivers/net/ethernet/alacritech/*
+
 ALCATEL SPEEDTOUCH USB DRIVER
 M:	Duncan Sands <duncan.sands@free.fr>
 L:	linux-usb@vger.kernel.org
-- 
1.9.1

[toc] | [prev] | [next] | [standalone]

#1530654

On 26.11.2016 13:20, Lino Sanfilippo wrote:

> v3:
> - dont add defines to pci.h but instead put it into the drivers header file

This should of course be "pci_ids.h".

Regards,
Lino

[toc] | [prev] | [next] | [standalone]

#1530676 — Re: [PATCH v3 net-next 1/2] net: ethernet: slicoss: add slicoss gigabit ethernet driver

Hi, Lino,

...

> @@ -0,0 +1,28 @@
> +config NET_VENDOR_ALACRITECH
> +        bool "Alacritech devices"
> +        default y
> +        ---help---
> +          If you have a network (Ethernet) card belonging to this class, say Y.
> +
> +          Note that the answer to this question doesn't directly affect the
> +          kernel: saying N will just cause the configurator to skip all

Shouldn't it be "Alacritech devices" here, as appears earlier ?

> +          the questions about Renesas devices. If you say Y, you will be asked
> +          for your specific device in the following questions.
> +

...
...
...
> +struct slic_device {
> +       struct pci_dev *pdev;
...
> +       bool promisc;

Seems that the autoneg boolean is not used anywhere, apart from
setting it once to true in
the slic_set_link_autoneg() method. Apart from this member it is not
accessed anywhere, so it seems it should be removed.

> +       bool autoneg;
> +       int speed;
...
...

> +static int slic_load_rcvseq_firmware(struct slic_device *sdev)
> +{
> +       const struct firmware *fw;
> +       const char *file;
> +       u32 codelen;
> +       int idx = 0;
> +       u32 instr;
> +       u32 addr;
> +       int err;
> +
...
> +       /* Do an initial sanity check concerning firmware size now. A further
> +        * check follows below.
> +        */
> +       if (fw->size < SLIC_FIRMWARE_MIN_SIZE) {
> +               dev_err(&sdev->pdev->dev,
> +                       "invalid firmware size %zu (min %u expected)\n",
> +                       fw->size, SLIC_FIRMWARE_MIN_SIZE);
> +               err = -EINVAL;

in the release label, always 0 is returned:

> +               goto release;
> +       }
> +
> +       codelen = slic_read_dword_from_firmware(fw, &idx);
> +
> +       /* do another sanity check against firmware size */
> +       if ((codelen + 4) > fw->size) {
> +               dev_err(&sdev->pdev->dev,
> +                       "invalid rcv-sequencer firmware size %zu\n", fw->size);
> +               err = -EINVAL;

Again, in the release label, always 0 is returned:

> +               goto release;
> +       }
> +
>
> +release:
> +       release_firmware(fw);
> +
> +       return 0;
> +}
> +

Regards,
Rami Rosen

[toc] | [prev] | [next] | [standalone]

#1530819 — Re: [PATCH v3 net-next 1/2] net: ethernet: slicoss: add slicoss gigabit ethernet driver

Hi Rami,


On 26.11.2016 16:48, Rami Rosen wrote:
>> @@ -0,0 +1,28 @@
>> +config NET_VENDOR_ALACRITECH
>> +        bool "Alacritech devices"
>> +        default y
>> +        ---help---
>> +          If you have a network (Ethernet) card belonging to this class, say Y.
>> +
>> +          Note that the answer to this question doesn't directly affect the
>> +          kernel: saying N will just cause the configurator to skip all
> 
> Shouldn't it be "Alacritech devices" here, as appears earlier ?
> 
>> +          the questions about Renesas devices. If you say Y, you will be asked

Yes, it definitely should not be Renesas :). This is a stupid copy and paste error, I will fix it,
thank you! 

>> +          for your specific device in the following questions.
>> +
> 
> ...
> ...
> ...
>> +struct slic_device {
>> +       struct pci_dev *pdev;
> ...
>> +       bool promisc;
> 
> Seems that the autoneg boolean is not used anywhere, apart from
> setting it once to true in
> the slic_set_link_autoneg() method. Apart from this member it is not
> accessed anywhere, so it seems it should be removed.
> 
>> +       bool autoneg;
>> +       int speed;

Agreed, this variable can be removed.

> ...
> 
>> +static int slic_load_rcvseq_firmware(struct slic_device *sdev)
>> +{
>> +       const struct firmware *fw;
>> +       const char *file;
>> +       u32 codelen;
>> +       int idx = 0;
>> +       u32 instr;
>> +       u32 addr;
>> +       int err;
>> +
> ...
>> +       /* Do an initial sanity check concerning firmware size now. A further
>> +        * check follows below.
>> +        */
>> +       if (fw->size < SLIC_FIRMWARE_MIN_SIZE) {
>> +               dev_err(&sdev->pdev->dev,
>> +                       "invalid firmware size %zu (min %u expected)\n",
>> +                       fw->size, SLIC_FIRMWARE_MIN_SIZE);
>> +               err = -EINVAL;
> 
> in the release label, always 0 is returned:
> 
>> +               goto release;
>> +       }
>> +
>> +       codelen = slic_read_dword_from_firmware(fw, &idx);
>> +
>> +       /* do another sanity check against firmware size */
>> +       if ((codelen + 4) > fw->size) {
>> +               dev_err(&sdev->pdev->dev,
>> +                       "invalid rcv-sequencer firmware size %zu\n", fw->size);
>> +               err = -EINVAL;
> 
> Again, in the release label, always 0 is returned:
> 
>> +               goto release;
>> +       }
>> +
>>
>> +release:
>> +       release_firmware(fw);
>> +
>> +       return 0;
>> +}

This should return "err", I will fix it.

Thanks a lot for the review!

Regards,
Lino

[toc] | [prev] | [next] | [standalone]

#1530914 — Re: [PATCH v3 net-next 1/2] net: ethernet: slicoss: add slicoss gigabit ethernet driver

Hello Lino,

just some things barely worth mentioning:

On 11/26/2016 01:20 PM, Lino Sanfilippo wrote:
> Add driver for Alacritech gigabit ethernet cards with SLIC (session-layer
> interface control) technology. The driver provides basic support without
> SLIC for the following devices:
> 
> - Mojave cards (single port PCI Gigabit) both copper and fiber
> - Oasis cards (single and dual port PCI-x Gigabit) copper and fiber
> - Kalahari cards (dual and quad port PCI-e Gigabit) copper and fiber
> 
> Signed-off-by: Lino Sanfilippo <LinoSanfilippo@gmx.de>
> ---
>  drivers/net/ethernet/Kconfig              |    1 +
>  drivers/net/ethernet/Makefile             |    1 +
>  drivers/net/ethernet/alacritech/Kconfig   |   28 +
>  drivers/net/ethernet/alacritech/Makefile  |    4 +
>  drivers/net/ethernet/alacritech/slic.h    |  576 +++++++++
>  drivers/net/ethernet/alacritech/slicoss.c | 1867 +++++++++++++++++++++++++++++
>  6 files changed, 2477 insertions(+)
>  create mode 100644 drivers/net/ethernet/alacritech/Kconfig
>  create mode 100644 drivers/net/ethernet/alacritech/Makefile
>  create mode 100644 drivers/net/ethernet/alacritech/slic.h
>  create mode 100644 drivers/net/ethernet/alacritech/slicoss.c
> 

[...]

> diff --git a/drivers/net/ethernet/alacritech/slic.h b/drivers/net/ethernet/alacritech/slic.h
> new file mode 100644
> index 0000000..c62d46b
> --- /dev/null
> +++ b/drivers/net/ethernet/alacritech/slic.h
> @@ -0,0 +1,576 @@
> +
> +#ifndef _SLIC_H
> +#define _SLIC_H


I found a bunch of unused #defines in slic.h. I cannot judge if they are
worth keeping:

	SLIC_VRHSTATB_LONGE
	SLIC_VRHSTATB_PREA
	SLIC_ISR_IO
	SLIC_ISR_PING_MASK
	SLIC_GIG_SPEED_MASK
	SLIC_GMCR_RESET
	SLIC_XCR_RESET
	SLIC_XCR_XMTEN
	SLIC_XCR_PAUSEEN
	SLIC_XCR_LOADRNG
	SLIC_REG_DBAR
	SLIC_REG_PING
	SLIC_REG_DUMP_CMD
	SLIC_REG_DUMP_DATA
	SLIC_REG_WRHOSTID
	SLIC_REG_LOW_POWER
	SLIC_REG_RESET_IFACE
	SLIC_REG_ADDR_UPPER
	SLIC_REG_HBAR64
	SLIC_REG_DBAR64
	SLIC_REG_CBAR64
	SLIC_REG_RBAR64
	SLIC_REG_WRVLANID
	SLIC_REG_READ_XF_INFO
	SLIC_REG_WRITE_XF_INFO
	SLIC_REG_TICKS_PER_SEC

These device IDs are not used, either, but maybe it's good to keep them
for documentation purposes:

	PCI_SUBDEVICE_ID_ALACRITECH_1000X1_2
	PCI_SUBDEVICE_ID_ALACRITECH_SES1001T
	PCI_SUBDEVICE_ID_ALACRITECH_SEN2002XT
	PCI_SUBDEVICE_ID_ALACRITECH_SEN2001XT
	PCI_SUBDEVICE_ID_ALACRITECH_SEN2104ET
	PCI_SUBDEVICE_ID_ALACRITECH_SEN2102ET

[...]

> +
> +/* SLIC EEPROM structure for Oasis */
> +struct slic_mojave_eeprom {

Comment: "for Mojave".

[...]

> +struct slic_device {
> +	struct pci_dev *pdev;
> +	struct net_device *netdev;
> +	void __iomem *regs;
> +	/* upper address setting lock */
> +	spinlock_t upper_lock;
> +	struct slic_shmem shmem;
> +	struct napi_struct napi;
> +	struct slic_rx_queue rxq;
> +	struct slic_tx_queue txq;
> +	struct slic_stat_queue stq;
> +	struct slic_stats stats;
> +	struct slic_upr_list upr_list;
> +	/* link configuration lock */
> +	spinlock_t link_lock;
> +	bool promisc;
> +	bool autoneg;
> +	int speed;
> +	int duplex;

Maybe make speed and duplex unsigned? They are assigned and compared
against unsigned values in slicoss.c, so this would get rid of some
(benign, because of the range of the values) -Wsign-compare warnings in
slic_configure_link_locked. However, in a comparison there SPEED_UNKNOWN
would need to be casted to unsigned to prevent another one popping up.

[...]

> +#endif /* _SLIC_H */
> diff --git a/drivers/net/ethernet/alacritech/slicoss.c b/drivers/net/ethernet/alacritech/slicoss.c
> new file mode 100644
> index 0000000..8cd862a
> --- /dev/null
> +++ b/drivers/net/ethernet/alacritech/slicoss.c
> @@ -0,0 +1,1867 @@

[...]

> +
> +static const struct pci_device_id slic_id_tbl[] = {
> +	{ PCI_DEVICE(PCI_VENDOR_ID_ALACRITECH,
> +		     PCI_DEVICE_ID_ALACRITECH_MOAVE) },

I missed this in slic.h, but is this a typo and "MOAVE" should be
"MOJAVE"? There are a couple similar #defines in slic.h.

[...]

> +static void slic_refill_rx_queue(struct slic_device *sdev, gfp_t gfp)
> +{
> +	const unsigned int ALIGN_MASK = SLIC_RX_BUFF_ALIGN - 1;
> +	unsigned int maplen = SLIC_RX_BUFF_SIZE;
> +	struct slic_rx_queue *rxq = &sdev->rxq;
> +	struct net_device *dev = sdev->netdev;
> +	struct slic_rx_buffer *buff;
> +	struct slic_rx_desc *desc;
> +	unsigned int misalign;
> +	unsigned int offset;
> +	struct sk_buff *skb;
> +	dma_addr_t paddr;
> +
> +	while (slic_get_free_rx_descs(rxq) > SLIC_MAX_REQ_RX_DESCS) {
> +		skb = alloc_skb(maplen + ALIGN_MASK, gfp);
> +		if (!skb)
> +			break;
> +
> +		paddr = dma_map_single(&sdev->pdev->dev, skb->data, maplen,
> +				       DMA_FROM_DEVICE);
> +		if (dma_mapping_error(&sdev->pdev->dev, paddr)) {
> +			netdev_err(dev, "mapping rx packet failed\n");
> +			/* drop skb */
> +			dev_kfree_skb_any(skb);
> +			break;
> +		}
> +		/* ensure head buffer descriptors are 256 byte aligned */
> +		offset = 0;
> +		misalign = paddr & ALIGN_MASK;
> +		if (misalign) {
> +			offset = SLIC_RX_BUFF_ALIGN - misalign;
> +			skb_reserve(skb, offset);
> +		}
> +		/* the HW expects dma chunks for descriptor + frame data */
> +		desc = (struct slic_rx_desc *)skb->data;
> +		memset(desc, 0, sizeof(*desc));
> +
> +		buff = &rxq->rxbuffs[rxq->put_idx];
> +		buff->skb = skb;
> +		dma_unmap_addr_set(buff, map_addr, paddr);
> +		dma_unmap_len_set(buff, map_len, maplen);
> +		buff->addr_offset = offset;
> +		/* head buffer descriptors are placed immediately before skb */
> +		slic_write(sdev, SLIC_REG_HBAR, lower_32_bits(paddr) +
> +						offset);

This fits nicely on one line. :-)

[...]

> +static int slic_init_tx_queue(struct slic_device *sdev)
> +{
> +	struct slic_tx_queue *txq = &sdev->txq;
> +	struct slic_tx_buffer *buff;
> +	struct slic_tx_desc *desc;
> +	int err;
> +	int i;

You could make i unsigned...

> +
> +	txq->len = SLIC_NUM_TX_DESCS;
> +	txq->put_idx = 0;
> +	txq->done_idx = 0;
> +
> +	txq->txbuffs = kcalloc(txq->len, sizeof(*buff), GFP_KERNEL);
> +	if (!txq->txbuffs)
> +		return -ENOMEM;
> +
> +	txq->dma_pool = dma_pool_create("slic_pool", &sdev->pdev->dev,
> +					sizeof(*desc), SLIC_TX_DESC_ALIGN,
> +					4096);
> +	if (!txq->dma_pool) {
> +		err = -ENOMEM;
> +		netdev_err(sdev->netdev, "failed to create dma pool\n");
> +		goto free_buffs;
> +	}
> +
> +	for (i = 0; i < txq->len; i++) {

...to fix a signed/unsigned comparison warning here, but...

> +		buff = &txq->txbuffs[i];
> +		desc = dma_pool_zalloc(txq->dma_pool, GFP_KERNEL,
> +				       &buff->desc_paddr);
> +		if (!desc) {
> +			netdev_err(sdev->netdev,
> +				   "failed to alloc pool chunk (%i)\n", i);
> +			err = -ENOMEM;
> +			goto free_descs;
> +		}
> +
> +		desc->hnd = cpu_to_le32((u32)(i + 1));
> +		desc->cmd = SLIC_CMD_XMT_REQ;
> +		desc->flags = 0;
> +		desc->type = cpu_to_le32(SLIC_CMD_TYPE_DUMB);
> +		buff->desc = desc;
> +	}
> +
> +	return 0;
> +
> +free_descs:
> +	while (i--) {

...this would require reworking this logic to prevent an endless loop,
so probably not worth bothering, considering that txq->len is well
within the positive signed range.

> +		buff = &txq->txbuffs[i];
> +		dma_pool_free(txq->dma_pool, buff->desc, buff->desc_paddr);
> +	}
> +	dma_pool_destroy(txq->dma_pool);
> +
> +free_buffs:
> +	kfree(txq->txbuffs);
> +
> +	return err;
> +}
> +
> +static void slic_free_tx_queue(struct slic_device *sdev)
> +{
> +	struct slic_tx_queue *txq = &sdev->txq;
> +	struct slic_tx_buffer *buff;
> +	int i;

Make i unsigned? One warning less, almost no work invested.

> +
> +	for (i = 0; i < txq->len; i++) {
> +		buff = &txq->txbuffs[i];
> +		dma_pool_free(txq->dma_pool, buff->desc, buff->desc_paddr);
> +		if (!buff->skb)
> +			continue;
> +
> +		dma_unmap_single(&sdev->pdev->dev,
> +				 dma_unmap_addr(buff, map_addr),
> +				 dma_unmap_len(buff, map_len), DMA_TO_DEVICE);
> +		consume_skb(buff->skb);
> +	}
> +	dma_pool_destroy(txq->dma_pool);
> +
> +	kfree(txq->txbuffs);
> +}
> +

[...]

> +static void slic_free_rx_queue(struct slic_device *sdev)
> +{
> +	struct slic_rx_queue *rxq = &sdev->rxq;
> +	struct slic_rx_buffer *buff;
> +	int i;

Unsigned?

> +
> +	/* free rx buffers */
> +	for (i = 0; i < rxq->len; i++) {
> +		buff = &rxq->rxbuffs[i];
> +
> +		if (!buff->skb)
> +			continue;
> +
> +		dma_unmap_single(&sdev->pdev->dev,
> +				 dma_unmap_addr(buff, map_addr),
> +				 dma_unmap_len(buff, map_len),
> +				 DMA_FROM_DEVICE);
> +		consume_skb(buff->skb);
> +	}
> +	kfree(rxq->rxbuffs);
> +}

[...]

> +static int slic_load_firmware(struct slic_device *sdev)
> +{
> +	u32 sectstart[SLIC_FIRMWARE_MAX_SECTIONS];
> +	u32 sectsize[SLIC_FIRMWARE_MAX_SECTIONS];
> +	const struct firmware *fw;
> +	unsigned int datalen;
> +	const char *file;
> +	int code_start;
> +	u32 numsects;
> +	int idx = 0;
> +	u32 sect;
> +	u32 instr;
> +	u32 addr;
> +	u32 base;
> +	int err;
> +	int i;

Make i unsigned?

> +
> +	file = (sdev->model == SLIC_MODEL_OASIS) ?  SLIC_FIRMWARE_OASIS :
> +						    SLIC_FIRMWARE_MOAVE;
> +	err = request_firmware(&fw, file, &sdev->pdev->dev);
> +	if (err) {
> +		dev_err(&sdev->pdev->dev, "failed to load firmware %s\n", file);
> +		return err;
> +	}
> +	/* Do an initial sanity check concerning firmware size now. A further
> +	 * check follows below.
> +	 */
> +	if (fw->size < SLIC_FIRMWARE_MIN_SIZE) {
> +		dev_err(&sdev->pdev->dev,
> +			"invalid firmware size %zu (min is %u)\n", fw->size,
> +			SLIC_FIRMWARE_MIN_SIZE);
> +		err = -EINVAL;
> +		goto release;
> +	}
> +
> +	numsects = slic_read_dword_from_firmware(fw, &idx);
> +	if (numsects == 0 || numsects > SLIC_FIRMWARE_MAX_SECTIONS) {
> +		dev_err(&sdev->pdev->dev,
> +			"invalid number of sections in firmware: %u", numsects);
> +		err = -EINVAL;
> +		goto release;
> +	}
> +
> +	datalen = numsects * 8 + 4;
> +	for (i = 0; i < numsects; i++) {
> +		sectsize[i] = slic_read_dword_from_firmware(fw, &idx);
> +		datalen += sectsize[i];
> +	}
> +
> +	/* do another sanity check against firmware size */
> +	if (datalen > fw->size) {
> +		dev_err(&sdev->pdev->dev,
> +			"invalid firmware size %zu (expected >= %u)\n",
> +			fw->size, datalen);
> +		err = -EINVAL;
> +		goto release;
> +	}
> +	/* get sections */
> +	for (i = 0; i < numsects; i++)
> +		sectstart[i] = slic_read_dword_from_firmware(fw, &idx);
> +
> +	code_start = idx;
> +	instr = slic_read_dword_from_firmware(fw, &idx);
> +
> +	for (sect = 0; sect < numsects; sect++) {
> +		unsigned int ssize = sectsize[sect] >> 3;
> +
> +		base = sectstart[sect];
> +
> +		for (addr = 0; addr < ssize; addr++) {
> +			/* write out instruction address */
> +			slic_write(sdev, SLIC_REG_WCS, base + addr);
> +			/* write out instruction to low addr */
> +			slic_write(sdev, SLIC_REG_WCS, instr);
> +			instr = slic_read_dword_from_firmware(fw, &idx);
> +			/* write out instruction to high addr */
> +			slic_write(sdev, SLIC_REG_WCS, instr);
> +			instr = slic_read_dword_from_firmware(fw, &idx);
> +		}
> +	}
> +
> +	idx = code_start;
> +
> +	for (sect = 0; sect < numsects; sect++) {
> +		unsigned int ssize = sectsize[sect] >> 3;
> +
> +		instr = slic_read_dword_from_firmware(fw, &idx);
> +		base = sectstart[sect];
> +		if (base < 0x8000)
> +			continue;
> +
> +		for (addr = 0; addr < ssize; addr++) {
> +			/* write out instruction address */
> +			slic_write(sdev, SLIC_REG_WCS,
> +				   SLIC_WCS_COMPARE | (base + addr));
> +			/* write out instruction to low addr */
> +			slic_write(sdev, SLIC_REG_WCS, instr);
> +			instr = slic_read_dword_from_firmware(fw, &idx);
> +			/* write out instruction to high addr */
> +			slic_write(sdev, SLIC_REG_WCS, instr);
> +			instr = slic_read_dword_from_firmware(fw, &idx);
> +		}
> +	}
> +	slic_flush_write(sdev);
> +	mdelay(10);
> +	/* everything OK, kick off the card */
> +	slic_write(sdev, SLIC_REG_WCS, SLIC_WCS_START);
> +	slic_flush_write(sdev);
> +	/* wait long enough for ucode to init card and reach the mainloop */
> +	mdelay(20);
> +release:
> +	release_firmware(fw);
> +
> +	return err;
> +}

[...]

> +static int slic_init_iface(struct slic_device *sdev)
> +{
> +	struct slic_shmem *sm = &sdev->shmem;
> +	int err;
> +
> +	sdev->upr_list.pending = false;
> +
> +	err = slic_init_shmem(sdev);
> +	if (err) {
> +		netdev_err(sdev->netdev, "failed to load firmware\n");

Wrong error message.

> +		return err;
> +	}

[...]

> +static netdev_tx_t slic_xmit(struct sk_buff *skb, struct net_device *dev)
> +{
> +	struct slic_device *sdev = netdev_priv(dev);
> +	struct slic_tx_queue *txq = &sdev->txq;
> +	struct slic_tx_buffer *buff;
> +	struct slic_tx_desc *desc;
> +	dma_addr_t paddr;
> +	u32 cbar_val;
> +	u32 maplen;
> +
> +	if (unlikely(slic_get_free_tx_descs(txq) < SLIC_MAX_REQ_TX_DESCS)) {
> +		netdev_err(dev, "BUG! not enought tx LEs left: %u\n",

"Enough"?

> +			   slic_get_free_tx_descs(txq));
> +		return NETDEV_TX_BUSY;
> +	}

[...]

> +static int slic_read_eeprom(struct slic_device *sdev)
> +{
> +	unsigned int devfn = PCI_FUNC(sdev->pdev->devfn);
> +	struct slic_shmem *sm = &sdev->shmem;
> +	struct slic_shmem_data *sm_data = sm->shmem_data;
> +	const unsigned int MAX_LOOPS = 5000;

Another benign -Wsign-compare warning can be fixed by either dropping
the unsigned here or making i below unsigned, too.

> +	unsigned int codesize;
> +	unsigned char *eeprom;
> +	struct slic_upr *upr;
> +	dma_addr_t paddr;
> +	int err = 0;
> +	u8 *mac[2];
> +	int i = 0;
> +
> +	eeprom = dma_zalloc_coherent(&sdev->pdev->dev, SLIC_EEPROM_SIZE,
> +				     &paddr, GFP_KERNEL);
> +	if (!eeprom)
> +		return -ENOMEM;
> +
> +	slic_write(sdev, SLIC_REG_ICR, SLIC_ICR_INT_OFF);
> +	/* setup ISP temporarily */
> +	slic_write(sdev, SLIC_REG_ISP, lower_32_bits(sm->isr_paddr));
> +
> +	err = slic_new_upr(sdev, SLIC_UPR_CONFIG, paddr);
> +	if (!err) {
> +		for (i = 0; i < MAX_LOOPS; i++) {
> +			if (le32_to_cpu(sm_data->isr) & SLIC_ISR_UPC)
> +				break;
> +			mdelay(1);
> +		}
> +		if (i == MAX_LOOPS) {
> +			dev_err(&sdev->pdev->dev,
> +				"timed out while waiting for eeprom data\n");
> +			err = -ETIMEDOUT;
> +		}
> +		upr = slic_dequeue_upr(sdev);
> +		kfree(upr);
> +	}
> +
> +	slic_write(sdev, SLIC_REG_ISP, 0);
> +	slic_write(sdev, SLIC_REG_ISR, 0);
> +	slic_flush_write(sdev);
> +
> +	if (err)
> +		goto free_eeprom;
> +
> +	if (sdev->model == SLIC_MODEL_OASIS) {
> +		struct slic_oasis_eeprom *oee;
> +
> +		oee = (struct slic_oasis_eeprom *)eeprom;
> +		mac[0] = oee->mac;
> +		mac[1] = oee->mac2;
> +		codesize = le16_to_cpu(oee->eeprom_code_size);
> +	} else {
> +		struct slic_mojave_eeprom *mee;
> +
> +		mee = (struct slic_mojave_eeprom *)eeprom;
> +		mac[0] = mee->mac;
> +		mac[1] = mee->mac2;
> +		codesize = le16_to_cpu(mee->eeprom_code_size);
> +	}
> +
> +	if (!slic_eeprom_valid(eeprom, codesize)) {
> +		dev_err(&sdev->pdev->dev, "invalid checksum in eeprom\n");
> +		err = -EINVAL;
> +		goto free_eeprom;
> +	}
> +	/* set mac address */
> +	ether_addr_copy(sdev->netdev->dev_addr, mac[devfn]);
> +free_eeprom:
> +	dma_free_coherent(&sdev->pdev->dev, SLIC_EEPROM_SIZE, eeprom, paddr);
> +
> +	return err;
> +}

[...]

> +static int slic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
> +{

[...]

> +	err = register_netdev(dev);
> +	if (err) {
> +		dev_err(&pdev->dev, "failed to register net device: %i\n",
> +			err);

Could be on one line.

Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1531651 — Re: [PATCH v3 net-next 1/2] net: ethernet: slicoss: add slicoss gigabit ethernet driver

Hi Markus,

On 27.11.2016 18:59, Markus Böhme wrote:
> Hello Lino,
> 
> just some things barely worth mentioning:
> 

> 
> I found a bunch of unused #defines in slic.h. I cannot judge if they are
> worth keeping:
> 
> 	SLIC_VRHSTATB_LONGE
> 	SLIC_VRHSTATB_PREA
> 	SLIC_ISR_IO
> 	SLIC_ISR_PING_MASK
> 	SLIC_GIG_SPEED_MASK
> 	SLIC_GMCR_RESET
> 	SLIC_XCR_RESET
> 	SLIC_XCR_XMTEN
> 	SLIC_XCR_PAUSEEN
> 	SLIC_XCR_LOADRNG
> 	SLIC_REG_DBAR
> 	SLIC_REG_PING
> 	SLIC_REG_DUMP_CMD
> 	SLIC_REG_DUMP_DATA
> 	SLIC_REG_WRHOSTID
> 	SLIC_REG_LOW_POWER
> 	SLIC_REG_RESET_IFACE
> 	SLIC_REG_ADDR_UPPER
> 	SLIC_REG_HBAR64
> 	SLIC_REG_DBAR64
> 	SLIC_REG_CBAR64
> 	SLIC_REG_RBAR64
> 	SLIC_REG_WRVLANID
> 	SLIC_REG_READ_XF_INFO
> 	SLIC_REG_WRITE_XF_INFO
> 	SLIC_REG_TICKS_PER_SEC
> 
> These device IDs are not used, either, but maybe it's good to keep them
> for documentation purposes:
> 
> 	PCI_SUBDEVICE_ID_ALACRITECH_1000X1_2
> 	PCI_SUBDEVICE_ID_ALACRITECH_SES1001T
> 	PCI_SUBDEVICE_ID_ALACRITECH_SEN2002XT
> 	PCI_SUBDEVICE_ID_ALACRITECH_SEN2001XT
> 	PCI_SUBDEVICE_ID_ALACRITECH_SEN2104ET
> 	PCI_SUBDEVICE_ID_ALACRITECH_SEN2102ET
> 

I left these defines in for both documentation and to avoid gaps in
register ranges. I would like to keep this as it is.

>> +
>> +/* SLIC EEPROM structure for Oasis */
>> +struct slic_mojave_eeprom {
> 
> Comment: "for Mojave".

Will fix, thanks,

> 
> [...]
> 
>> +struct slic_device {
>> +	struct pci_dev *pdev;
>> +	struct net_device *netdev;
>> +	void __iomem *regs;
>> +	/* upper address setting lock */
>> +	spinlock_t upper_lock;
>> +	struct slic_shmem shmem;
>> +	struct napi_struct napi;
>> +	struct slic_rx_queue rxq;
>> +	struct slic_tx_queue txq;
>> +	struct slic_stat_queue stq;
>> +	struct slic_stats stats;
>> +	struct slic_upr_list upr_list;
>> +	/* link configuration lock */
>> +	spinlock_t link_lock;
>> +	bool promisc;
>> +	bool autoneg;
>> +	int speed;
>> +	int duplex;
> 
> Maybe make speed and duplex unsigned? They are assigned and compared
> against unsigned values in slicoss.c, so this would get rid of some
> (benign, because of the range of the values) -Wsign-compare warnings in
> slic_configure_link_locked. However, in a comparison there SPEED_UNKNOWN
> would need to be casted to unsigned to prevent another one popping up.
> 

There is indeed a bunch of warnings concerning signedness. Will have a look
at all of them. However I think I will keep "speed" as an int, because casting
SPEED_UNKNOWN to an unsigned int is IMHO an ugly thing to do.

> [...]
> 
>> +#endif /* _SLIC_H */
>> diff --git a/drivers/net/ethernet/alacritech/slicoss.c b/drivers/net/ethernet/alacritech/slicoss.c
>> new file mode 100644
>> index 0000000..8cd862a
>> --- /dev/null
>> +++ b/drivers/net/ethernet/alacritech/slicoss.c
>> @@ -0,0 +1,1867 @@
> 
> [...]
> 
>> +
>> +static const struct pci_device_id slic_id_tbl[] = {
>> +	{ PCI_DEVICE(PCI_VENDOR_ID_ALACRITECH,
>> +		     PCI_DEVICE_ID_ALACRITECH_MOAVE) },
> 
> I missed this in slic.h, but is this a typo and "MOAVE" should be
> "MOJAVE"? There are a couple similar #defines in slic.h.

This should definitely be "Mojave". Will fix it. 

> 
> [...]
> 
>> +static void slic_refill_rx_queue(struct slic_device *sdev, gfp_t gfp)
>> +{
>> +	const unsigned int ALIGN_MASK = SLIC_RX_BUFF_ALIGN - 1;
>> +	unsigned int maplen = SLIC_RX_BUFF_SIZE;
>> +	struct slic_rx_queue *rxq = &sdev->rxq;
>> +	struct net_device *dev = sdev->netdev;
>> +	struct slic_rx_buffer *buff;
>> +	struct slic_rx_desc *desc;
>> +	unsigned int misalign;
>> +	unsigned int offset;
>> +	struct sk_buff *skb;
>> +	dma_addr_t paddr;
>> +
>> +	while (slic_get_free_rx_descs(rxq) > SLIC_MAX_REQ_RX_DESCS) {
>> +		skb = alloc_skb(maplen + ALIGN_MASK, gfp);
>> +		if (!skb)
>> +			break;
>> +
>> +		paddr = dma_map_single(&sdev->pdev->dev, skb->data, maplen,
>> +				       DMA_FROM_DEVICE);
>> +		if (dma_mapping_error(&sdev->pdev->dev, paddr)) {
>> +			netdev_err(dev, "mapping rx packet failed\n");
>> +			/* drop skb */
>> +			dev_kfree_skb_any(skb);
>> +			break;
>> +		}
>> +		/* ensure head buffer descriptors are 256 byte aligned */
>> +		offset = 0;
>> +		misalign = paddr & ALIGN_MASK;
>> +		if (misalign) {
>> +			offset = SLIC_RX_BUFF_ALIGN - misalign;
>> +			skb_reserve(skb, offset);
>> +		}
>> +		/* the HW expects dma chunks for descriptor + frame data */
>> +		desc = (struct slic_rx_desc *)skb->data;
>> +		memset(desc, 0, sizeof(*desc));
>> +
>> +		buff = &rxq->rxbuffs[rxq->put_idx];
>> +		buff->skb = skb;
>> +		dma_unmap_addr_set(buff, map_addr, paddr);
>> +		dma_unmap_len_set(buff, map_len, maplen);
>> +		buff->addr_offset = offset;
>> +		/* head buffer descriptors are placed immediately before skb */
>> +		slic_write(sdev, SLIC_REG_HBAR, lower_32_bits(paddr) +
>> +						offset);
> 
> This fits nicely on one line. :-)

Right, will fix.

> 
> [...]
> 
>> +static int slic_init_tx_queue(struct slic_device *sdev)
>> +{
>> +	struct slic_tx_queue *txq = &sdev->txq;
>> +	struct slic_tx_buffer *buff;
>> +	struct slic_tx_desc *desc;
>> +	int err;
>> +	int i;
> 
> You could make i unsigned...
> 

>> +
>> +	txq->len = SLIC_NUM_TX_DESCS;
>> +	txq->put_idx = 0;
>> +	txq->done_idx = 0;
>> +
>> +	txq->txbuffs = kcalloc(txq->len, sizeof(*buff), GFP_KERNEL);
>> +	if (!txq->txbuffs)
>> +		return -ENOMEM;
>> +
>> +	txq->dma_pool = dma_pool_create("slic_pool", &sdev->pdev->dev,
>> +					sizeof(*desc), SLIC_TX_DESC_ALIGN,
>> +					4096);
>> +	if (!txq->dma_pool) {
>> +		err = -ENOMEM;
>> +		netdev_err(sdev->netdev, "failed to create dma pool\n");
>> +		goto free_buffs;
>> +	}
>> +
>> +	for (i = 0; i < txq->len; i++) {
> 
> ...to fix a signed/unsigned comparison warning here, but...
> 
>> +		buff = &txq->txbuffs[i];
>> +		desc = dma_pool_zalloc(txq->dma_pool, GFP_KERNEL,
>> +				       &buff->desc_paddr);
>> +		if (!desc) {
>> +			netdev_err(sdev->netdev,
>> +				   "failed to alloc pool chunk (%i)\n", i);
>> +			err = -ENOMEM;
>> +			goto free_descs;
>> +		}
>> +
>> +		desc->hnd = cpu_to_le32((u32)(i + 1));
>> +		desc->cmd = SLIC_CMD_XMT_REQ;
>> +		desc->flags = 0;
>> +		desc->type = cpu_to_le32(SLIC_CMD_TYPE_DUMB);
>> +		buff->desc = desc;
>> +	}
>> +
>> +	return 0;
>> +
>> +free_descs:
>> +	while (i--) {
> 
> ...this would require reworking this logic to prevent an endless loop,
> so probably not worth bothering, considering that txq->len is well
> within the positive signed range.

AFAICS the logic does not have to be changed. The while loop will also work
fine if "i" is unsigned.

> 
>> +		buff = &txq->txbuffs[i];
>> +		dma_pool_free(txq->dma_pool, buff->desc, buff->desc_paddr);
>> +	}
>> +	dma_pool_destroy(txq->dma_pool);
>> +
>> +free_buffs:
>> +	kfree(txq->txbuffs);
>> +
>> +	return err;
>> +}
>> +
>> +static void slic_free_tx_queue(struct slic_device *sdev)
>> +{
>> +	struct slic_tx_queue *txq = &sdev->txq;
>> +	struct slic_tx_buffer *buff;
>> +	int i;
> 
> Make i unsigned? One warning less, almost no work invested.
> 
>> +
>> +	for (i = 0; i < txq->len; i++) {
>> +		buff = &txq->txbuffs[i];
>> +		dma_pool_free(txq->dma_pool, buff->desc, buff->desc_paddr);
>> +		if (!buff->skb)
>> +			continue;
>> +
>> +		dma_unmap_single(&sdev->pdev->dev,
>> +				 dma_unmap_addr(buff, map_addr),
>> +				 dma_unmap_len(buff, map_len), DMA_TO_DEVICE);
>> +		consume_skb(buff->skb);
>> +	}
>> +	dma_pool_destroy(txq->dma_pool);
>> +
>> +	kfree(txq->txbuffs);
>> +}
>> +
> 
> [...]
> 
>> +static void slic_free_rx_queue(struct slic_device *sdev)
>> +{
>> +	struct slic_rx_queue *rxq = &sdev->rxq;
>> +	struct slic_rx_buffer *buff;
>> +	int i;
> 
> Unsigned?
> 
>> +
>> +	/* free rx buffers */
>> +	for (i = 0; i < rxq->len; i++) {
>> +		buff = &rxq->rxbuffs[i];
>> +
>> +		if (!buff->skb)
>> +			continue;
>> +
>> +		dma_unmap_single(&sdev->pdev->dev,
>> +				 dma_unmap_addr(buff, map_addr),
>> +				 dma_unmap_len(buff, map_len),
>> +				 DMA_FROM_DEVICE);
>> +		consume_skb(buff->skb);
>> +	}
>> +	kfree(rxq->rxbuffs);
>> +}
> 
> [...]
> 
>> +static int slic_load_firmware(struct slic_device *sdev)
>> +{
>> +	u32 sectstart[SLIC_FIRMWARE_MAX_SECTIONS];
>> +	u32 sectsize[SLIC_FIRMWARE_MAX_SECTIONS];
>> +	const struct firmware *fw;
>> +	unsigned int datalen;
>> +	const char *file;
>> +	int code_start;
>> +	u32 numsects;
>> +	int idx = 0;
>> +	u32 sect;
>> +	u32 instr;
>> +	u32 addr;
>> +	u32 base;
>> +	int err;
>> +	int i;
> 
> Make i unsigned?
> 
>> +
>> +	file = (sdev->model == SLIC_MODEL_OASIS) ?  SLIC_FIRMWARE_OASIS :
>> +						    SLIC_FIRMWARE_MOAVE;
>> +	err = request_firmware(&fw, file, &sdev->pdev->dev);
>> +	if (err) {
>> +		dev_err(&sdev->pdev->dev, "failed to load firmware %s\n", file);
>> +		return err;
>> +	}
>> +	/* Do an initial sanity check concerning firmware size now. A further
>> +	 * check follows below.
>> +	 */
>> +	if (fw->size < SLIC_FIRMWARE_MIN_SIZE) {
>> +		dev_err(&sdev->pdev->dev,
>> +			"invalid firmware size %zu (min is %u)\n", fw->size,
>> +			SLIC_FIRMWARE_MIN_SIZE);
>> +		err = -EINVAL;
>> +		goto release;
>> +	}
>> +
>> +	numsects = slic_read_dword_from_firmware(fw, &idx);
>> +	if (numsects == 0 || numsects > SLIC_FIRMWARE_MAX_SECTIONS) {
>> +		dev_err(&sdev->pdev->dev,
>> +			"invalid number of sections in firmware: %u", numsects);
>> +		err = -EINVAL;
>> +		goto release;
>> +	}
>> +
>> +	datalen = numsects * 8 + 4;
>> +	for (i = 0; i < numsects; i++) {
>> +		sectsize[i] = slic_read_dword_from_firmware(fw, &idx);
>> +		datalen += sectsize[i];
>> +	}
>> +
>> +	/* do another sanity check against firmware size */
>> +	if (datalen > fw->size) {
>> +		dev_err(&sdev->pdev->dev,
>> +			"invalid firmware size %zu (expected >= %u)\n",
>> +			fw->size, datalen);
>> +		err = -EINVAL;
>> +		goto release;
>> +	}
>> +	/* get sections */
>> +	for (i = 0; i < numsects; i++)
>> +		sectstart[i] = slic_read_dword_from_firmware(fw, &idx);
>> +
>> +	code_start = idx;
>> +	instr = slic_read_dword_from_firmware(fw, &idx);
>> +
>> +	for (sect = 0; sect < numsects; sect++) {
>> +		unsigned int ssize = sectsize[sect] >> 3;
>> +
>> +		base = sectstart[sect];
>> +
>> +		for (addr = 0; addr < ssize; addr++) {
>> +			/* write out instruction address */
>> +			slic_write(sdev, SLIC_REG_WCS, base + addr);
>> +			/* write out instruction to low addr */
>> +			slic_write(sdev, SLIC_REG_WCS, instr);
>> +			instr = slic_read_dword_from_firmware(fw, &idx);
>> +			/* write out instruction to high addr */
>> +			slic_write(sdev, SLIC_REG_WCS, instr);
>> +			instr = slic_read_dword_from_firmware(fw, &idx);
>> +		}
>> +	}
>> +
>> +	idx = code_start;
>> +
>> +	for (sect = 0; sect < numsects; sect++) {
>> +		unsigned int ssize = sectsize[sect] >> 3;
>> +
>> +		instr = slic_read_dword_from_firmware(fw, &idx);
>> +		base = sectstart[sect];
>> +		if (base < 0x8000)
>> +			continue;
>> +
>> +		for (addr = 0; addr < ssize; addr++) {
>> +			/* write out instruction address */
>> +			slic_write(sdev, SLIC_REG_WCS,
>> +				   SLIC_WCS_COMPARE | (base + addr));
>> +			/* write out instruction to low addr */
>> +			slic_write(sdev, SLIC_REG_WCS, instr);
>> +			instr = slic_read_dword_from_firmware(fw, &idx);
>> +			/* write out instruction to high addr */
>> +			slic_write(sdev, SLIC_REG_WCS, instr);
>> +			instr = slic_read_dword_from_firmware(fw, &idx);
>> +		}
>> +	}
>> +	slic_flush_write(sdev);
>> +	mdelay(10);
>> +	/* everything OK, kick off the card */
>> +	slic_write(sdev, SLIC_REG_WCS, SLIC_WCS_START);
>> +	slic_flush_write(sdev);
>> +	/* wait long enough for ucode to init card and reach the mainloop */
>> +	mdelay(20);
>> +release:
>> +	release_firmware(fw);
>> +
>> +	return err;
>> +}
> 
> [...]
> 
>> +static int slic_init_iface(struct slic_device *sdev)
>> +{
>> +	struct slic_shmem *sm = &sdev->shmem;
>> +	int err;
>> +
>> +	sdev->upr_list.pending = false;
>> +
>> +	err = slic_init_shmem(sdev);
>> +	if (err) {
>> +		netdev_err(sdev->netdev, "failed to load firmware\n");
> 
> Wrong error message.

Yep, will fix.

> 
>> +		return err;
>> +	}
> 
> [...]
> 
>> +static netdev_tx_t slic_xmit(struct sk_buff *skb, struct net_device *dev)
>> +{
>> +	struct slic_device *sdev = netdev_priv(dev);
>> +	struct slic_tx_queue *txq = &sdev->txq;
>> +	struct slic_tx_buffer *buff;
>> +	struct slic_tx_desc *desc;
>> +	dma_addr_t paddr;
>> +	u32 cbar_val;
>> +	u32 maplen;
>> +
>> +	if (unlikely(slic_get_free_tx_descs(txq) < SLIC_MAX_REQ_TX_DESCS)) {
>> +		netdev_err(dev, "BUG! not enought tx LEs left: %u\n",
> 
> "Enough"?

Will fix.

>> +			   slic_get_free_tx_descs(txq));
>> +		return NETDEV_TX_BUSY;
>> +	}
> 
> [...]
> 
>> +static int slic_read_eeprom(struct slic_device *sdev)
>> +{
>> +	unsigned int devfn = PCI_FUNC(sdev->pdev->devfn);
>> +	struct slic_shmem *sm = &sdev->shmem;
>> +	struct slic_shmem_data *sm_data = sm->shmem_data;
>> +	const unsigned int MAX_LOOPS = 5000;
> 
> Another benign -Wsign-compare warning can be fixed by either dropping
> the unsigned here or making i below unsigned, too.
> 
>> +	unsigned int codesize;
>> +	unsigned char *eeprom;
>> +	struct slic_upr *upr;
>> +	dma_addr_t paddr;
>> +	int err = 0;
>> +	u8 *mac[2];
>> +	int i = 0;
>> +
>> +	eeprom = dma_zalloc_coherent(&sdev->pdev->dev, SLIC_EEPROM_SIZE,
>> +				     &paddr, GFP_KERNEL);
>> +	if (!eeprom)
>> +		return -ENOMEM;
>> +
>> +	slic_write(sdev, SLIC_REG_ICR, SLIC_ICR_INT_OFF);
>> +	/* setup ISP temporarily */
>> +	slic_write(sdev, SLIC_REG_ISP, lower_32_bits(sm->isr_paddr));
>> +
>> +	err = slic_new_upr(sdev, SLIC_UPR_CONFIG, paddr);
>> +	if (!err) {
>> +		for (i = 0; i < MAX_LOOPS; i++) {
>> +			if (le32_to_cpu(sm_data->isr) & SLIC_ISR_UPC)
>> +				break;
>> +			mdelay(1);
>> +		}
>> +		if (i == MAX_LOOPS) {
>> +			dev_err(&sdev->pdev->dev,
>> +				"timed out while waiting for eeprom data\n");
>> +			err = -ETIMEDOUT;
>> +		}
>> +		upr = slic_dequeue_upr(sdev);
>> +		kfree(upr);
>> +	}
>> +
>> +	slic_write(sdev, SLIC_REG_ISP, 0);
>> +	slic_write(sdev, SLIC_REG_ISR, 0);
>> +	slic_flush_write(sdev);
>> +
>> +	if (err)
>> +		goto free_eeprom;
>> +
>> +	if (sdev->model == SLIC_MODEL_OASIS) {
>> +		struct slic_oasis_eeprom *oee;
>> +
>> +		oee = (struct slic_oasis_eeprom *)eeprom;
>> +		mac[0] = oee->mac;
>> +		mac[1] = oee->mac2;
>> +		codesize = le16_to_cpu(oee->eeprom_code_size);
>> +	} else {
>> +		struct slic_mojave_eeprom *mee;
>> +
>> +		mee = (struct slic_mojave_eeprom *)eeprom;
>> +		mac[0] = mee->mac;
>> +		mac[1] = mee->mac2;
>> +		codesize = le16_to_cpu(mee->eeprom_code_size);
>> +	}
>> +
>> +	if (!slic_eeprom_valid(eeprom, codesize)) {
>> +		dev_err(&sdev->pdev->dev, "invalid checksum in eeprom\n");
>> +		err = -EINVAL;
>> +		goto free_eeprom;
>> +	}
>> +	/* set mac address */
>> +	ether_addr_copy(sdev->netdev->dev_addr, mac[devfn]);
>> +free_eeprom:
>> +	dma_free_coherent(&sdev->pdev->dev, SLIC_EEPROM_SIZE, eeprom, paddr);
>> +
>> +	return err;
>> +}
> 
> [...]
> 
>> +static int slic_probe(struct pci_dev *pdev, const struct pci_device_id *ent)
>> +{
> 
> [...]
> 
>> +	err = register_netdev(dev);
>> +	if (err) {
>> +		dev_err(&pdev->dev, "failed to register net device: %i\n",
>> +			err);
> 
> Could be on one line.

Right, will adjust it.


> Regards,
> Markus
> 

Thanks Markus!

Regards,
Lino

[toc] | [prev] | [next] | [standalone]

#1532234 — Re: [PATCH v3 net-next 1/2] net: ethernet: slicoss: add slicoss gigabit ethernet driver

On 11/28/2016 09:46 PM, Lino Sanfilippo wrote:
> Hi Markus,
> 
> On 27.11.2016 18:59, Markus Böhme wrote:
>> Hello Lino,
>>
>> just some things barely worth mentioning:
>>
> 
>>
>> I found a bunch of unused #defines in slic.h. I cannot judge if they are
>> worth keeping:
>>
>> 	SLIC_VRHSTATB_LONGE
>> 	SLIC_VRHSTATB_PREA
>> 	SLIC_ISR_IO
>> 	SLIC_ISR_PING_MASK
>> 	SLIC_GIG_SPEED_MASK
>> 	SLIC_GMCR_RESET
>> 	SLIC_XCR_RESET
>> 	SLIC_XCR_XMTEN
>> 	SLIC_XCR_PAUSEEN
>> 	SLIC_XCR_LOADRNG
>> 	SLIC_REG_DBAR
>> 	SLIC_REG_PING
>> 	SLIC_REG_DUMP_CMD
>> 	SLIC_REG_DUMP_DATA
>> 	SLIC_REG_WRHOSTID
>> 	SLIC_REG_LOW_POWER
>> 	SLIC_REG_RESET_IFACE
>> 	SLIC_REG_ADDR_UPPER
>> 	SLIC_REG_HBAR64
>> 	SLIC_REG_DBAR64
>> 	SLIC_REG_CBAR64
>> 	SLIC_REG_RBAR64
>> 	SLIC_REG_WRVLANID
>> 	SLIC_REG_READ_XF_INFO
>> 	SLIC_REG_WRITE_XF_INFO
>> 	SLIC_REG_TICKS_PER_SEC
>>
>> These device IDs are not used, either, but maybe it's good to keep them
>> for documentation purposes:
>>
>> 	PCI_SUBDEVICE_ID_ALACRITECH_1000X1_2
>> 	PCI_SUBDEVICE_ID_ALACRITECH_SES1001T
>> 	PCI_SUBDEVICE_ID_ALACRITECH_SEN2002XT
>> 	PCI_SUBDEVICE_ID_ALACRITECH_SEN2001XT
>> 	PCI_SUBDEVICE_ID_ALACRITECH_SEN2104ET
>> 	PCI_SUBDEVICE_ID_ALACRITECH_SEN2102ET
>>
> 
> I left these defines in for both documentation and to avoid gaps in
> register ranges. I would like to keep this as it is.

Seems reasonable.

[...]

>>> +static int slic_init_tx_queue(struct slic_device *sdev)
>>> +{
>>> +	struct slic_tx_queue *txq = &sdev->txq;
>>> +	struct slic_tx_buffer *buff;
>>> +	struct slic_tx_desc *desc;
>>> +	int err;
>>> +	int i;
>>
>> You could make i unsigned...
>>
> 
>>> +
>>> +	txq->len = SLIC_NUM_TX_DESCS;
>>> +	txq->put_idx = 0;
>>> +	txq->done_idx = 0;
>>> +
>>> +	txq->txbuffs = kcalloc(txq->len, sizeof(*buff), GFP_KERNEL);
>>> +	if (!txq->txbuffs)
>>> +		return -ENOMEM;
>>> +
>>> +	txq->dma_pool = dma_pool_create("slic_pool", &sdev->pdev->dev,
>>> +					sizeof(*desc), SLIC_TX_DESC_ALIGN,
>>> +					4096);
>>> +	if (!txq->dma_pool) {
>>> +		err = -ENOMEM;
>>> +		netdev_err(sdev->netdev, "failed to create dma pool\n");
>>> +		goto free_buffs;
>>> +	}
>>> +
>>> +	for (i = 0; i < txq->len; i++) {
>>
>> ...to fix a signed/unsigned comparison warning here, but...
>>
>>> +		buff = &txq->txbuffs[i];
>>> +		desc = dma_pool_zalloc(txq->dma_pool, GFP_KERNEL,
>>> +				       &buff->desc_paddr);
>>> +		if (!desc) {
>>> +			netdev_err(sdev->netdev,
>>> +				   "failed to alloc pool chunk (%i)\n", i);
>>> +			err = -ENOMEM;
>>> +			goto free_descs;
>>> +		}
>>> +
>>> +		desc->hnd = cpu_to_le32((u32)(i + 1));
>>> +		desc->cmd = SLIC_CMD_XMT_REQ;
>>> +		desc->flags = 0;
>>> +		desc->type = cpu_to_le32(SLIC_CMD_TYPE_DUMB);
>>> +		buff->desc = desc;
>>> +	}
>>> +
>>> +	return 0;
>>> +
>>> +free_descs:
>>> +	while (i--) {
>>
>> ...this would require reworking this logic to prevent an endless loop,
>> so probably not worth bothering, considering that txq->len is well
>> within the positive signed range.
> 
> AFAICS the logic does not have to be changed. The while loop will also work
> fine if "i" is unsigned.
> 

My bad! Of course you are right.


Regards,
Markus

[toc] | [prev] | [next] | [standalone]

#1531034 — Re: [PATCH v3 net-next 1/2] net: ethernet: slicoss: add slicoss gigabit ethernet driver

On 11/26/2016 04:20 AM, Lino Sanfilippo wrote:
> Add driver for Alacritech gigabit ethernet cards with SLIC (session-layer
> interface control) technology. The driver provides basic support without
> SLIC for the following devices:
> 
> - Mojave cards (single port PCI Gigabit) both copper and fiber
> - Oasis cards (single and dual port PCI-x Gigabit) copper and fiber
> - Kalahari cards (dual and quad port PCI-e Gigabit) copper and fiber

This looks great, a few nits below:


> +#define SLIC_MAX_TX_COMPLETIONS		100

You usually don't want to limit the number of TX completion, if the
entire TX ring needs to be cleaned, you would want to allow that.

[snip]

> +	while (slic_get_free_rx_descs(rxq) > SLIC_MAX_REQ_RX_DESCS) {
> +		skb = alloc_skb(maplen + ALIGN_MASK, gfp);
> +		if (!skb)
> +			break;
> +
> +		paddr = dma_map_single(&sdev->pdev->dev, skb->data, maplen,
> +				       DMA_FROM_DEVICE);
> +		if (dma_mapping_error(&sdev->pdev->dev, paddr)) {
> +			netdev_err(dev, "mapping rx packet failed\n");
> +			/* drop skb */
> +			dev_kfree_skb_any(skb);
> +			break;
> +		}
> +		/* ensure head buffer descriptors are 256 byte aligned */
> +		offset = 0;
> +		misalign = paddr & ALIGN_MASK;
> +		if (misalign) {
> +			offset = SLIC_RX_BUFF_ALIGN - misalign;
> +			skb_reserve(skb, offset);
> +		}
> +		/* the HW expects dma chunks for descriptor + frame data */
> +		desc = (struct slic_rx_desc *)skb->data;
> +		memset(desc, 0, sizeof(*desc));

Do you really need to zero-out the prepending RX descriptor? Are not you
missing a write barrier here?

[snip]

> +
> +		dma_sync_single_for_cpu(&sdev->pdev->dev,
> +					dma_unmap_addr(buff, map_addr),
> +					buff->addr_offset + sizeof(*desc),
> +					DMA_FROM_DEVICE);
> +
> +		status = le32_to_cpu(desc->status);
> +		if (!(status & SLIC_IRHDDR_SVALID))
> +			break;
> +
> +		buff->skb = NULL;
> +
> +		dma_unmap_single(&sdev->pdev->dev,
> +				 dma_unmap_addr(buff, map_addr),
> +				 dma_unmap_len(buff, map_len),
> +				 DMA_FROM_DEVICE);

This is potentially inefficient, you already did a cache invalidation
for the RX descriptor here, you could be more efficient with just
invalidating the packet length, minus the descriptor length.

> +
> +		/* skip rx descriptor that is placed before the frame data */
> +		skb_reserve(skb, SLIC_RX_BUFF_HDR_SIZE);
> +
> +		if (unlikely(status & SLIC_IRHDDR_ERR)) {
> +			slic_handle_frame_error(sdev, skb);
> +			dev_kfree_skb_any(skb);
> +		} else {
> +			struct ethhdr *eh = (struct ethhdr *)skb->data;
> +
> +			if (is_multicast_ether_addr(eh->h_dest))
> +				SLIC_INC_STATS_COUNTER(&sdev->stats, rx_mcasts);
> +
> +			len = le32_to_cpu(desc->length) & SLIC_IRHDDR_FLEN_MSK;
> +			skb_put(skb, len);
> +			skb->protocol = eth_type_trans(skb, dev);
> +			skb->ip_summed = CHECKSUM_UNNECESSARY;
> +			skb->dev = dev;

eth_type_trans() already assigns skb->dev = dev;

> +static int slic_poll(struct napi_struct *napi, int todo)
> +{
> +	struct slic_device *sdev = container_of(napi, struct slic_device, napi);
> +	struct slic_shmem *sm = &sdev->shmem;
> +	struct slic_shmem_data *sm_data = sm->shmem_data;
> +	u32 isr = le32_to_cpu(sm_data->isr);
> +	unsigned int done = 0;
> +
> +	slic_handle_irq(sdev, isr, todo, &done);
> +
> +	if (done < todo) {
> +		napi_complete(napi);

napi_complete_done() since you know how many packets you completed.

> +		/* reenable irqs */
> +		sm_data->isr = 0;
> +		/* make sure sm_data->isr is cleard before irqs are reenabled */
> +		wmb();
> +		slic_write(sdev, SLIC_REG_ISR, 0);
> +		slic_flush_write(sdev);
> +	}
> +
> +	return done;
> +}
> +
> +static irqreturn_t slic_irq(int irq, void *dev_id)
> +{
> +	struct slic_device *sdev = dev_id;
> +	struct slic_shmem *sm = &sdev->shmem;
> +	struct slic_shmem_data *sm_data = sm->shmem_data;
> +
> +	slic_write(sdev, SLIC_REG_ICR, SLIC_ICR_INT_MASK);
> +	slic_flush_write(sdev);
> +	/* make sure sm_data->isr is read after ICR_INT_MASK is set */
> +	wmb();
> +
> +	if (!sm_data->isr) {
> +		dma_rmb();
> +		/* spurious interrupt */
> +		slic_write(sdev, SLIC_REG_ISR, 0);
> +		slic_flush_write(sdev);
> +		return IRQ_NONE;
> +	}
> +
> +	napi_schedule(&sdev->napi);

Likewise napi_schedule_irqoff() can be used here.
-- 
Florian

[toc] | [prev] | [next] | [standalone]

#1531713 — Re: [PATCH v3 net-next 1/2] net: ethernet: slicoss: add slicoss gigabit ethernet driver

Hi Florian,

On 28.11.2016 05:56, Florian Fainelli wrote:
> On 11/26/2016 04:20 AM, Lino Sanfilippo wrote:
>> Add driver for Alacritech gigabit ethernet cards with SLIC (session-layer
>> interface control) technology. The driver provides basic support without
>> SLIC for the following devices:
>> 
>> - Mojave cards (single port PCI Gigabit) both copper and fiber
>> - Oasis cards (single and dual port PCI-x Gigabit) copper and fiber
>> - Kalahari cards (dual and quad port PCI-e Gigabit) copper and fiber
> 
> This looks great, a few nits below:
> 
> 
>> +#define SLIC_MAX_TX_COMPLETIONS		100
> 
> You usually don't want to limit the number of TX completion, if the
> entire TX ring needs to be cleaned, you would want to allow that.
> 

The problem is that the HW does not provide a tx completion index. Instead we have to 
iterate the status descriptors until we get an invalid idx which indicates that there 
are no further tx descriptors done for now. I am afraid that if we do not limit the 
number of descriptors processed in the tx completion handler, a continuous transmission 
of frames could keep the loop in xmit_complete() run endlessly. I dont know if this 
can actually happen but I wanted to make sure that this is avoided.

> [snip]
> 
>> +	while (slic_get_free_rx_descs(rxq) > SLIC_MAX_REQ_RX_DESCS) {
>> +		skb = alloc_skb(maplen + ALIGN_MASK, gfp);
>> +		if (!skb)
>> +			break;
>> +
>> +		paddr = dma_map_single(&sdev->pdev->dev, skb->data, maplen,
>> +				       DMA_FROM_DEVICE);
>> +		if (dma_mapping_error(&sdev->pdev->dev, paddr)) {
>> +			netdev_err(dev, "mapping rx packet failed\n");
>> +			/* drop skb */
>> +			dev_kfree_skb_any(skb);
>> +			break;
>> +		}
>> +		/* ensure head buffer descriptors are 256 byte aligned */
>> +		offset = 0;
>> +		misalign = paddr & ALIGN_MASK;
>> +		if (misalign) {
>> +			offset = SLIC_RX_BUFF_ALIGN - misalign;
>> +			skb_reserve(skb, offset);
>> +		}
>> +		/* the HW expects dma chunks for descriptor + frame data */
>> +		desc = (struct slic_rx_desc *)skb->data;
>> +		memset(desc, 0, sizeof(*desc));
> 
> Do you really need to zero-out the prepending RX descriptor? Are not you
> missing a write barrier here?

Indeed, it should be sufficient to make sure that the bit SLIC_IRHDDR_SVALID is not set.
I will adjust it. 
Concerning the write barrier: You mean a wmb() before slic_write() to ensure that the zeroing
 of the status desc is done before the descriptor is passed to the HW, right?


> [snip]
> 
>> +
>> +		dma_sync_single_for_cpu(&sdev->pdev->dev,
>> +					dma_unmap_addr(buff, map_addr),
>> +					buff->addr_offset + sizeof(*desc),
>> +					DMA_FROM_DEVICE);
>> +
>> +		status = le32_to_cpu(desc->status);
>> +		if (!(status & SLIC_IRHDDR_SVALID))
>> +			break;
>> +
>> +		buff->skb = NULL;
>> +
>> +		dma_unmap_single(&sdev->pdev->dev,
>> +				 dma_unmap_addr(buff, map_addr),
>> +				 dma_unmap_len(buff, map_len),
>> +				 DMA_FROM_DEVICE);
> 
> This is potentially inefficient, you already did a cache invalidation
> for the RX descriptor here, you could be more efficient with just
> invalidating the packet length, minus the descriptor length.
> 

I am not sure I understand: We have to unmap the complete dma area, no matter if we synced
part of it before, dont we? AFAIK a dma sync is different from unmapping dma, or do I miss
something?


>> +
>> +		/* skip rx descriptor that is placed before the frame data */
>> +		skb_reserve(skb, SLIC_RX_BUFF_HDR_SIZE);
>> +
>> +		if (unlikely(status & SLIC_IRHDDR_ERR)) {
>> +			slic_handle_frame_error(sdev, skb);
>> +			dev_kfree_skb_any(skb);
>> +		} else {
>> +			struct ethhdr *eh = (struct ethhdr *)skb->data;
>> +
>> +			if (is_multicast_ether_addr(eh->h_dest))
>> +				SLIC_INC_STATS_COUNTER(&sdev->stats, rx_mcasts);
>> +
>> +			len = le32_to_cpu(desc->length) & SLIC_IRHDDR_FLEN_MSK;
>> +			skb_put(skb, len);
>> +			skb->protocol = eth_type_trans(skb, dev);
>> +			skb->ip_summed = CHECKSUM_UNNECESSARY;
>> +			skb->dev = dev;
> 
> eth_type_trans() already assigns skb->dev = dev;
> 

Right, this is unnecessary, I will fix it.

>> +static int slic_poll(struct napi_struct *napi, int todo)
>> +{
>> +	struct slic_device *sdev = container_of(napi, struct slic_device, napi);
>> +	struct slic_shmem *sm = &sdev->shmem;
>> +	struct slic_shmem_data *sm_data = sm->shmem_data;
>> +	u32 isr = le32_to_cpu(sm_data->isr);
>> +	unsigned int done = 0;
>> +
>> +	slic_handle_irq(sdev, isr, todo, &done);
>> +
>> +	if (done < todo) {
>> +		napi_complete(napi);
> 
> napi_complete_done() since you know how many packets you completed.
> 

Ok, will change it.

>> +		/* reenable irqs */
>> +		sm_data->isr = 0;
>> +		/* make sure sm_data->isr is cleard before irqs are reenabled */
>> +		wmb();
>> +		slic_write(sdev, SLIC_REG_ISR, 0);
>> +		slic_flush_write(sdev);
>> +	}
>> +
>> +	return done;
>> +}
>> +
>> +static irqreturn_t slic_irq(int irq, void *dev_id)
>> +{
>> +	struct slic_device *sdev = dev_id;
>> +	struct slic_shmem *sm = &sdev->shmem;
>> +	struct slic_shmem_data *sm_data = sm->shmem_data;
>> +
>> +	slic_write(sdev, SLIC_REG_ICR, SLIC_ICR_INT_MASK);
>> +	slic_flush_write(sdev);
>> +	/* make sure sm_data->isr is read after ICR_INT_MASK is set */
>> +	wmb();
>> +
>> +	if (!sm_data->isr) {
>> +		dma_rmb();
>> +		/* spurious interrupt */
>> +		slic_write(sdev, SLIC_REG_ISR, 0);
>> +		slic_flush_write(sdev);
>> +		return IRQ_NONE;
>> +	}
>> +
>> +	napi_schedule(&sdev->napi);
> 
> Likewise napi_schedule_irqoff() can be used here.
> 

Ok, will change it.

Thanks a lot Florian!

Regards,
Lino

[toc] | [prev] | [next] | [standalone]

#1532560 — Re: [PATCH v3 net-next 1/2] net: ethernet: slicoss: add slicoss gigabit ethernet driver

On 11/28/2016 01:41 PM, Lino Sanfilippo wrote:
> The problem is that the HW does not provide a tx completion index. Instead we have to 
> iterate the status descriptors until we get an invalid idx which indicates that there 
> are no further tx descriptors done for now. I am afraid that if we do not limit the 
> number of descriptors processed in the tx completion handler, a continuous transmission 
> of frames could keep the loop in xmit_complete() run endlessly. I dont know if this 
> can actually happen but I wanted to make sure that this is avoided.

OK, it might be a good idea to put that comment somewhere around the tx
completion handler to understand why it is bounded with a specific value.

> 
>> [snip]
>>
>>> +	while (slic_get_free_rx_descs(rxq) > SLIC_MAX_REQ_RX_DESCS) {
>>> +		skb = alloc_skb(maplen + ALIGN_MASK, gfp);
>>> +		if (!skb)
>>> +			break;
>>> +
>>> +		paddr = dma_map_single(&sdev->pdev->dev, skb->data, maplen,
>>> +				       DMA_FROM_DEVICE);
>>> +		if (dma_mapping_error(&sdev->pdev->dev, paddr)) {
>>> +			netdev_err(dev, "mapping rx packet failed\n");
>>> +			/* drop skb */
>>> +			dev_kfree_skb_any(skb);
>>> +			break;
>>> +		}
>>> +		/* ensure head buffer descriptors are 256 byte aligned */
>>> +		offset = 0;
>>> +		misalign = paddr & ALIGN_MASK;
>>> +		if (misalign) {
>>> +			offset = SLIC_RX_BUFF_ALIGN - misalign;
>>> +			skb_reserve(skb, offset);
>>> +		}
>>> +		/* the HW expects dma chunks for descriptor + frame data */
>>> +		desc = (struct slic_rx_desc *)skb->data;
>>> +		memset(desc, 0, sizeof(*desc));
>>
>> Do you really need to zero-out the prepending RX descriptor? Are not you
>> missing a write barrier here?
> 
> Indeed, it should be sufficient to make sure that the bit SLIC_IRHDDR_SVALID is not set.
> I will adjust it. 
> Concerning the write barrier: You mean a wmb() before slic_write() to ensure that the zeroing
>  of the status desc is done before the descriptor is passed to the HW, right?

Correct, that's what I meant here.

> 
> 
>> [snip]
>>
>>> +
>>> +		dma_sync_single_for_cpu(&sdev->pdev->dev,
>>> +					dma_unmap_addr(buff, map_addr),
>>> +					buff->addr_offset + sizeof(*desc),
>>> +					DMA_FROM_DEVICE);
>>> +
>>> +		status = le32_to_cpu(desc->status);
>>> +		if (!(status & SLIC_IRHDDR_SVALID))
>>> +			break;
>>> +
>>> +		buff->skb = NULL;
>>> +
>>> +		dma_unmap_single(&sdev->pdev->dev,
>>> +				 dma_unmap_addr(buff, map_addr),
>>> +				 dma_unmap_len(buff, map_len),
>>> +				 DMA_FROM_DEVICE);
>>
>> This is potentially inefficient, you already did a cache invalidation
>> for the RX descriptor here, you could be more efficient with just
>> invalidating the packet length, minus the descriptor length.
>>
> 
> I am not sure I understand: We have to unmap the complete dma area, no matter if we synced
> part of it before, dont we? AFAIK a dma sync is different from unmapping dma, or do I miss
> something?

Sorry, I was not very clear, what I meant is that you can allocate and
do the initial dma_map_single() of your RX skbs during ndo_open(), and
then, in your RX path, you can only do dma_sync_single_for_cpu() twice
(once for the RX descriptor status, second time for the actual packet
contents), and when you return the SKB to the HW, do a
dma_sync_single_for_device(). The advantage of doing that, is that if
your cache operations are slow, you only do them on exactly packet
length, and not the actual RX buffer size (e.g: 2KB).
-- 
Florian

[toc] | [prev] | [next] | [standalone]

#1533537 — Re: [PATCH v3 net-next 1/2] net: ethernet: slicoss: add slicoss gigabit ethernet driver

On 29.11.2016 18:14, Florian Fainelli wrote:
> On 11/28/2016 01:41 PM, Lino Sanfilippo wrote:
>> The problem is that the HW does not provide a tx completion index. Instead we have to 
>> iterate the status descriptors until we get an invalid idx which indicates that there 
>> are no further tx descriptors done for now. I am afraid that if we do not limit the 
>> number of descriptors processed in the tx completion handler, a continuous transmission 
>> of frames could keep the loop in xmit_complete() run endlessly. I dont know if this 
>> can actually happen but I wanted to make sure that this is avoided.
> 
> OK, it might be a good idea to put that comment somewhere around the tx
> completion handler to understand why it is bounded with a specific value.
> 

Agreed, I will add such a comment.

>> 
>>> [snip]
>>>
>>>> +	while (slic_get_free_rx_descs(rxq) > SLIC_MAX_REQ_RX_DESCS) {
>>>> +		skb = alloc_skb(maplen + ALIGN_MASK, gfp);
>>>> +		if (!skb)
>>>> +			break;
>>>> +
>>>> +		paddr = dma_map_single(&sdev->pdev->dev, skb->data, maplen,
>>>> +				       DMA_FROM_DEVICE);
>>>> +		if (dma_mapping_error(&sdev->pdev->dev, paddr)) {
>>>> +			netdev_err(dev, "mapping rx packet failed\n");
>>>> +			/* drop skb */
>>>> +			dev_kfree_skb_any(skb);
>>>> +			break;
>>>> +		}
>>>> +		/* ensure head buffer descriptors are 256 byte aligned */
>>>> +		offset = 0;
>>>> +		misalign = paddr & ALIGN_MASK;
>>>> +		if (misalign) {
>>>> +			offset = SLIC_RX_BUFF_ALIGN - misalign;
>>>> +			skb_reserve(skb, offset);
>>>> +		}
>>>> +		/* the HW expects dma chunks for descriptor + frame data */
>>>> +		desc = (struct slic_rx_desc *)skb->data;
>>>> +		memset(desc, 0, sizeof(*desc));
>>>
>>> Do you really need to zero-out the prepending RX descriptor? Are not you
>>> missing a write barrier here?
>> 
>> Indeed, it should be sufficient to make sure that the bit SLIC_IRHDDR_SVALID is not set.
>> I will adjust it. 
>> Concerning the write barrier: You mean a wmb() before slic_write() to ensure that the zeroing
>>  of the status desc is done before the descriptor is passed to the HW, right?
> 
> Correct, that's what I meant here.
> 

Ok, will fix this. Good catch BTW!

>> 
>>> [snip]
>>>
>>>> +
>>>> +		dma_sync_single_for_cpu(&sdev->pdev->dev,
>>>> +					dma_unmap_addr(buff, map_addr),
>>>> +					buff->addr_offset + sizeof(*desc),
>>>> +					DMA_FROM_DEVICE);
>>>> +
>>>> +		status = le32_to_cpu(desc->status);
>>>> +		if (!(status & SLIC_IRHDDR_SVALID))
>>>> +			break;
>>>> +
>>>> +		buff->skb = NULL;
>>>> +
>>>> +		dma_unmap_single(&sdev->pdev->dev,
>>>> +				 dma_unmap_addr(buff, map_addr),
>>>> +				 dma_unmap_len(buff, map_len),
>>>> +				 DMA_FROM_DEVICE);
>>>
>>> This is potentially inefficient, you already did a cache invalidation
>>> for the RX descriptor here, you could be more efficient with just
>>> invalidating the packet length, minus the descriptor length.
>>>
>> 
>> I am not sure I understand: We have to unmap the complete dma area, no matter if we synced
>> part of it before, dont we? AFAIK a dma sync is different from unmapping dma, or do I miss
>> something?
> 
> Sorry, I was not very clear, what I meant is that you can allocate and
> do the initial dma_map_single() of your RX skbs during ndo_open(), and
> then, in your RX path, you can only do dma_sync_single_for_cpu() twice
> (once for the RX descriptor status, second time for the actual packet
> contents), and when you return the SKB to the HW, do a
> dma_sync_single_for_device(). The advantage of doing that, is that if
> your cache operations are slow, you only do them on exactly packet
> length, and not the actual RX buffer size (e.g: 2KB).

Um. In the rx path the SKB will be consumed (by napi_gro_receive()).
AFAIK we _have_ to unmap it before this call. Doing only a dma_sync_single_for_cpu()
for the packet contents does IMHO only make sense if the corresponding SKB is
reused somehow. But this is not the case. The rx buffers are refilled with newly
allocated SKBs each time, and thus we need to create a new dma mapping for each of them.

Or do I still misunderstand when to call the dma sync functions?


BTW: I just realized that if the descriptor has not been used by the HW yet, see:

+		dma_sync_single_for_cpu(&sdev->pdev->dev,
+					dma_unmap_addr(buff, map_addr),
+					buff->addr_offset + sizeof(*desc),
+					DMA_FROM_DEVICE);
+
+		status = le32_to_cpu(desc->status);
+		if (!(status & SLIC_IRHDDR_SVALID))
+			break;
+		  
		^^^^^^^^^^^^^^^^^^^^^^^^^^^  dma_sync_single_for_device missing


there has to be a dma_sync_single_for_device to undo the sync for cpu (since the
HW will write to this descr when the next rx packet arrives), right? But this is racy:
What if the HW writes to that descr after we synced it for cpu but before we synced
it for the HW again? Any ideas?

Regards,
Lino

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web