lkml.org 
[lkml]   [2016]   [Nov]   [28]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
From
SubjectRe: [PATCH v3 net-next 1/2] net: ethernet: slicoss: add slicoss gigabit ethernet driver
Date
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

\
 
 \ /
  Last update: 2016-11-28 21:48    [W:0.265 / U:0.028 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site