On 11/28/2016 09:46 PM, Lino Sanfilippo wrote:

Hi Markus,

On 27.11.2016 18:59, Markus Böhme wrote:

quoted

Hello Lino,

just some things barely worth mentioning:

quoted

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.

[...]

quoted

quoted

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

quoted

quoted

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

quoted

+		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