Re: New commands to configure IOV features

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Fri, Sep 21, 2012 at 10:35 AM, Ben Hutchings
<bhutchings@xxxxxxxxxxxxxx> wrote:
> On Thu, 2012-09-20 at 22:50 -0700, Yinghai Lu wrote:
> [...]
>> Index: linux-2.6/include/linux/pci.h
>> ===================================================================
>> --- linux-2.6.orig/include/linux/pci.h
>> +++ linux-2.6/include/linux/pci.h
>> @@ -663,6 +663,7 @@ struct pci_driver {
>>         const struct pci_device_id *id_table;   /* must be non-NULL for probe to be called */
>>         int  (*probe)  (struct pci_dev *dev, const struct pci_device_id *id);   /* New device inserted */
>>         void (*remove) (struct pci_dev *dev);   /* Device removed (NULL if not a hot-plug capable driver) */
>> +       void (*set_max_vfs) (struct pci_dev *dev); /* enable sriov */
>>         int  (*suspend) (struct pci_dev *dev, pm_message_t state);      /* Device suspended */
>>         int  (*suspend_late) (struct pci_dev *dev, pm_message_t state);
>>         int  (*resume_early) (struct pci_dev *dev);
>
> Not sure about this; the driver may fail to enable those VFs and it
> would be nice to be able to report that failure directly.
>
> That said, this is 'max_vfs' (a limit) and if the driver fails to set up
> all the VFs then the *limit* may still be changed.
>
>> Subject: [PATCH] PCI: Add max_vfs in sysfs per pci device where supports
>>
>> driver later need to check dev->max_vfs in /sys
>>
>> Signed-off-by: Yinghai Lu <yinghai@xxxxxxxxxx>
> [...]
>> +static ssize_t
>> +max_vfs_store(struct device *dev, struct device_attribute *attr,
>> +                const char *buf, size_t count)
>> +{
>> +       unsigned long val;
>> +       struct pci_dev *pdev = to_pci_dev(dev);
>> +
>> +       if (strict_strtoul(buf, 0, &val) < 0)
>> +               return -EINVAL;
>> +
>> +       pdev->max_vfs = val;
>> +
>> +       if (pdev->is_added) {
>
> No locking required here?

should be removed.

>
>> +               int err;
>> +               err = device_schedule_callback(dev, max_vfs_callback);
>
> Any particular reason this should be a callback?

no, just copied that from bus rescan.

>
> [...]
>> Index: linux-2.6/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> ===================================================================
>> --- linux-2.6.orig/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> +++ linux-2.6/drivers/net/ethernet/intel/ixgbe/ixgbe_main.c
>> @@ -129,13 +129,6 @@ static struct notifier_block dca_notifie
>>  };
>>  #endif
>>
>> -#ifdef CONFIG_PCI_IOV
>> -static unsigned int max_vfs;
>> -module_param(max_vfs, uint, 0);
>> -MODULE_PARM_DESC(max_vfs,
>> -                "Maximum number of virtual functions to allocate per
>> physical function - default is zero and maximum value is 63");
>> -#endif /* CONFIG_PCI_IOV */
>> -
>>  static unsigned int allow_unsupported_sfp;
>>  module_param(allow_unsupported_sfp, uint, 0);
>>  MODULE_PARM_DESC(allow_unsupported_sfp,
>> @@ -4528,7 +4521,7 @@ static int __devinit ixgbe_sw_init(struc
>>  #ifdef CONFIG_PCI_IOV
>>         /* assign number of SR-IOV VFs */
>>         if (hw->mac.type != ixgbe_mac_82598EB)
>> -               adapter->num_vfs = (max_vfs > 63) ? 0 : max_vfs;
>> +               adapter->num_vfs = (pdev->max_vfs > 63) ? 0 : pdev->max_vfs;
>
> We are trying to make all SR-IOV capable drivers work the same, so this
> weird limiting behaviour should not be retained.
>
> So I think the correct assignment is:
>                 adapter->num_vfs = min(pdev->max_vfs, 63);

this will treat >63 as 63. old one treat > 63 as 0 aka disabled.

looks your version is more reasonable...

>
>>  #endif
>>         /* enable itr by default in dynamic mode */
>> @@ -7249,8 +7242,9 @@ static int __devinit ixgbe_probe(struct
>>
>>  #ifdef CONFIG_PCI_IOV
>>         ixgbe_enable_sriov(adapter, ii);
>> -
>>  #endif
>> +       adapter->ixgbe_info = ii;
>> +
>>         netdev->features = NETIF_F_SG |
>>                            NETIF_F_IP_CSUM |
>>                            NETIF_F_IPV6_CSUM |
>> @@ -7720,11 +7714,42 @@ static const struct pci_error_handlers i
>>         .resume = ixgbe_io_resume,
>>  };
>>
>> +static void ixgbe_set_max_vfs(struct pci_dev *pdev)
>> +{
>> +#ifdef CONFIG_PCI_IOV
>> +       struct ixgbe_adapter *adapter = pci_get_drvdata(pdev);
>> +       struct net_device *netdev = adapter->netdev;
>> +       struct ixgbe_hw *hw = &adapter->hw;
>> +       int num_vfs = 0;
>> +
>> +       /* assign number of SR-IOV VFs */
>> +       if (hw->mac.type != ixgbe_mac_82598EB)
>> +               num_vfs = (pdev->max_vfs > 63) ? 0 : pdev->max_vfs;
>> +
>> +       /* no change */
>> +       if (adapter->num_vfs == num_vfs)
>> +               return;
>> +
>> +       if (!num_vfs) {
>> +               /* disable sriov */
>> +               ixgbe_disable_sriov(adapter);
>> +               adapter->num_vfs = 0;
>> +       } else if (!adapter->num_vfs && num_vfs) {
>> +               /* enable sriov */
>> +               adapter->num_vfs = num_vfs;
>> +               ixgbe_enable_sriov(adapter, adapter->ixgbe_info);
>> +       } else {
>> +               /* increase or decrease */
>
> Indeed, increase or decrease is not supported either in our PCI API or
> in the SR-IOV spec.  I think I would prefer the PCI core to filter out
> unsupported changes (i.e. not call set_max_vfs and maybe report an
> error), but I'm not sure about that.

should still call set_max_vfs, and let it set finally valid max_vfs.

pci driver should know better which max_vfs is better for exact ...

-Yinghai
--
To unsubscribe from this list: send the line "unsubscribe linux-pci" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [DMA Engine]     [Linux Coverity]     [Linux USB]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [Greybus]

  Powered by Linux