netdev - Re: [PATCH v2 14/17] octeontx2-pf: Add basic ethtool support

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]

Message-ID: <CA+sq2Cc+8bgT8FsvFMOic0TiStgrOGsrA5b0nt84hVS3CVZ7TQ@mail.gmail.com>
Date:   Tue, 14 Jan 2020 15:56:23 +0530
From:   Sunil Kovvuri <sunil.kovvuri@...il.com>
To:     Michal Kubecek <mkubecek@...e.cz>
Cc:     Linux Netdev List <netdev@...r.kernel.org>,
        "David S. Miller" <davem@...emloft.net>,
        Jakub Kicinski <kubakici@...pl>,
        Christina Jacob <cjacob@...vell.com>,
        Sunil Goutham <sgoutham@...vell.com>
Subject: Re: [PATCH v2 14/17] octeontx2-pf: Add basic ethtool support

On Tue, Jan 14, 2020 at 3:38 PM Michal Kubecek <mkubecek@...e.cz> wrote:
>
> On Tue, Jan 14, 2020 at 12:32:17PM +0530, sunil.kovvuri@...il.com wrote:
> > From: Christina Jacob <cjacob@...vell.com>
> >
> > This patch adds ethtool support for
> >  - Driver stats, Tx/Rx perqueue and CGX LMAC stats
> >  - Set/show Rx/Tx queue count
> >  - Set/show Rx/Tx ring sizes
> >  - Set/show IRQ coalescing parameters
> >
> > Signed-off-by: Christina Jacob <cjacob@...vell.com>
> > Signed-off-by: Sunil Goutham <sgoutham@...vell.com>
> > ---
> [...]
> > +static void otx2_dev_open(struct net_device *netdev)
> > +{
> > +     otx2_open(netdev);
> > +}
> > +
> > +static void otx2_dev_stop(struct net_device *netdev)
> > +{
> > +     otx2_stop(netdev);
> > +}
>
> Why don't you call these directly?

Will submit VF driver as a follow up, which will result in calling
either otx2_stop or otx2vf_stop.

>
> [...]
> > +/* Get no of queues device supports and current queue count */
> > +static void otx2_get_channels(struct net_device *dev,
> > +                           struct ethtool_channels *channel)
> > +{
> > +     struct otx2_nic *pfvf = netdev_priv(dev);
> > +
> > +     memset(channel, 0, sizeof(*channel));
>
> The structure is already zero initialized in ethtool_get_channels()
> (except for cmd).

Will fix.

>
> > +     channel->max_rx = pfvf->hw.max_queues;
> > +     channel->max_tx = pfvf->hw.max_queues;
> > +
> > +     channel->rx_count = pfvf->hw.rx_queues;
> > +     channel->tx_count = pfvf->hw.tx_queues;
> > +}
> > +
> > +/* Set no of Tx, Rx queues to be used */
> > +static int otx2_set_channels(struct net_device *dev,
> > +                          struct ethtool_channels *channel)
> > +{
> > +     struct otx2_nic *pfvf = netdev_priv(dev);
> > +     bool if_up = netif_running(dev);
> > +     int err = 0;
> > +
> > +     if (!channel->rx_count || !channel->tx_count)
> > +             return -EINVAL;
> > +     if (channel->rx_count > pfvf->hw.max_queues)
> > +             return -EINVAL;
> > +     if (channel->tx_count > pfvf->hw.max_queues)
> > +             return -EINVAL;
>
> The upper bounds are checked in ethtool_set_channels() so that you don't
> get here if requested counts are too high.
>

Will fix.

> > +
> > +     if (if_up)
> > +             otx2_dev_stop(dev);
> > +
> > +     pfvf->hw.rx_queues = channel->rx_count;
> > +     pfvf->hw.tx_queues = channel->tx_count;
> > +     err = otx2_set_real_num_queues(dev, pfvf->hw.tx_queues,
> > +                                    pfvf->hw.rx_queues);
> > +     pfvf->qset.cq_cnt = pfvf->hw.tx_queues +  pfvf->hw.rx_queues;
> > +     if (err)
> > +             return err;
> > +
> > +     if (if_up)
> > +             otx2_dev_open(dev);
>
> Is it intentional that you leave the device down when the change fails?
>
> > +     netdev_info(dev, "Setting num Tx rings to %d, Rx rings to %d success\n",
> > +                 pfvf->hw.tx_queues, pfvf->hw.rx_queues);
> > +
> > +     return err;
> > +}
> > +
> > +static void otx2_get_ringparam(struct net_device *netdev,
> > +                            struct ethtool_ringparam *ring)
> > +{
> > +     struct otx2_nic *pfvf = netdev_priv(netdev);
> > +     struct otx2_qset *qs = &pfvf->qset;
> > +
> > +     ring->rx_max_pending = Q_COUNT(Q_SIZE_MAX);
> > +     ring->rx_pending = qs->rqe_cnt ? qs->rqe_cnt : Q_COUNT(Q_SIZE_256);
> > +     ring->tx_max_pending = Q_COUNT(Q_SIZE_MAX);
> > +     ring->tx_pending = qs->sqe_cnt ? qs->sqe_cnt : Q_COUNT(Q_SIZE_4K);
> > +}
> > +
> > +static int otx2_set_ringparam(struct net_device *netdev,
> > +                           struct ethtool_ringparam *ring)
> > +{
> > +     struct otx2_nic *pfvf = netdev_priv(netdev);
> > +     bool if_up = netif_running(netdev);
> > +     struct otx2_qset *qs = &pfvf->qset;
> > +     u32 rx_count, tx_count;
> > +
> > +     if (ring->rx_mini_pending || ring->rx_jumbo_pending)
> > +             return -EINVAL;
> > +
> > +     /* Permitted lengths are 16 64 256 1K 4K 16K 64K 256K 1M  */
> > +     rx_count = clamp_t(u32, ring->rx_pending,
> > +                        Q_COUNT(Q_SIZE_MIN), Q_COUNT(Q_SIZE_MAX));
>
> The upper bound is checked in ethtool_set_ringparam().
>
> Michal Kubecek
>

Will fix.
Thanks for the feedback.

Sunil.