netdev - Re: [PATCH net-next 3/6] octeontx2-vf: Virtual function driver dupport

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 11 Mar 2020 12:44:00 +0530
From:   Sunil Kovvuri <sunil.kovvuri@...il.com>
To:     Jakub Kicinski <kuba@...nel.org>
Cc:     Linux Netdev List <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Tomasz Duszynski <tduszynski@...vell.com>,
        Subbaraya Sundeep <sbhatta@...vell.com>,
        Geetha sowjanya <gakula@...vell.com>,
        Sunil Goutham <sgoutham@...vell.com>
Subject: Re: [PATCH net-next 3/6] octeontx2-vf: Virtual function driver dupport

On Wed, Mar 11, 2020 at 3:13 AM Jakub Kicinski <kuba@...nel.org> wrote:
>
> On Wed, 11 Mar 2020 00:17:22 +0530 sunil.kovvuri@...il.com wrote:
> > +#define DRV_NAME     "octeontx2-nicvf"
> > +#define DRV_STRING   "Marvell OcteonTX2 NIC Virtual Function Driver"
> > +#define DRV_VERSION  "1.0"
>
> Please drop the driver version, kernel version should be used upstream.
>

Okay, will do.

> > +
> > +static const struct pci_device_id otx2_vf_id_table[] = {
> > +     { PCI_DEVICE(PCI_VENDOR_ID_CAVIUM, PCI_DEVID_OCTEONTX2_RVU_AFVF) },
> > +     { PCI_DEVICE(PCI_VENDOR_ID_CAVIUM, PCI_DEVID_OCTEONTX2_RVU_VF) },
> > +     { }
> > +};
> > +
> > +MODULE_AUTHOR("Marvell International Ltd.");
>
> Only people can be authors, please put your name here or remove this.
>

Just for my understanding, is this due to a decision taken in netdev recently ?
I have searched through all drivers in netdev and there is a mix of
organizations and individuals as AUTHORS.
Here we used org name to avoid specifying multiple names.


> > +static netdev_tx_t otx2vf_xmit(struct sk_buff *skb, struct net_device *netdev)
> > +{
> > +     /* Check for minimum and maximum packet length */
> > +     if (skb->len <= ETH_HLEN ||
> > +         (!skb_shinfo(skb)->gso_size && skb->len > vf->max_frs)) {
>
> These should never happen (if they do we need to fix the stack, not
> sprinkle all the drivers with checks like this).
>

HW has limitation on minimum number of bytes being transmitted, hence the check.
Even i wasn't sure if it would be possible, but saw similar
limitation/checks in few other drivers, hence added.

Regarding maximum length check, i did see pkts with > MTU (when
pathMTU is considered).
But anyways due to changes patch6 of this series HW should shouldn't
complain, i will remove this.
Should have removed earlier itself, sorry for the trouble.

>
> > +static void otx2vf_reset_task(struct work_struct *work)
> > +{
> > +     struct otx2_nic *vf = container_of(work, struct otx2_nic, reset_task);
> > +
> > +     if (!netif_running(vf->netdev))
> > +             return;
> > +
> > +     otx2vf_stop(vf->netdev);
> > +     vf->reset_count++;
> > +     otx2vf_open(vf->netdev);
>
> What's the locking around here? Can user call open/stop while this is running?
>

Yes, possible, thanks, will fix.

> > +     netif_trans_update(vf->netdev);
> > +}
>
> > +static int otx2vf_realloc_msix_vectors(struct otx2_nic *vf)
> > +{
> > +     struct otx2_hw *hw = &vf->hw;
> > +     int num_vec, err;
> > +
> > +     num_vec = hw->nix_msixoff;
> > +     num_vec += NIX_LF_CINT_VEC_START + hw->max_queues;
> > +
> > +     otx2vf_disable_mbox_intr(vf);
> > +     pci_free_irq_vectors(hw->pdev);
> > +     pci_free_irq_vectors(hw->pdev);
>
> Why free IRQs twice?
>

hmm.. will fix.

> > +     err = otx2vf_register_mbox_intr(vf, false);
> > +     if (err)
> > +             return err;
>
> return otx2vf_re..

Okay, will change.

Thanks,
Sunil.