Re: [dts] [PATCH 3/4] framework: reorganize DUT and Tester port initialize sequence

["Liu, Yong" <yong.liu@intel.com>]

Michael, thanks for your comments.

> -----Original Message-----
> From: Qiu, Michael
> Sent: Wednesday, February 04, 2015 11:56 AM
> To: Liu, Yong; dts@dpdk.org
> Subject: Re: [dts] [PATCH 3/4] framework: reorganize DUT and Tester port
> initialize sequence
> 
> On 1/23/2015 4:27 PM, Marvin Liu wrote:
> > From: Yong Liu <yong.liu@intel.com>
> >
> > DUT port initialization sequence: scan,restore,rescan,load config
> > Test port initialization sequence: modprobe, interface up
> > Separate restore interface function from DUT and Tester.
> >
> > Signed-off-by: Marvinliu <yong.liu@intel.com>
> > ---
> >  framework/crb.py          |  53 ++++----------
> >  framework/dut.py          | 181 +++++++++++++++++++++++++++++++++++----
> -------
> >  framework/project_dpdk.py |  22 ++----
> >  framework/tester.py       |  30 ++++++++
> >  4 files changed, 190 insertions(+), 96 deletions(-)
> >
> 
> [...]
> 
> > @@ -494,3 +531,61 @@ class Dut(Crb):
> >              # store the port info to port mapping
> >              self.ports_info.append({'pci': pci_str, 'type': pci_id,
> 'intf':
> >                                      intf, 'mac': macaddr, 'ipv6': ipv6,
> 'numa': -1})
> > +
> > +    def restore_interfaces(self):
> > +        """
> > +        Restore Linux interfaces.
> > +        """
> > +        for port in self.ports_info:
> > +            pci_bus = port['pci']
> > +            pci_id = port['type']
> > +            # get device driver
> > +            driver = dts.get_nic_driver(pci_id)
> > +            if driver is not None:
> > +                # unbind device driver
> > +                addr_array = pci_bus.split(':')
> > +                bus_id = addr_array[0]
> > +                devfun_id = addr_array[1]
> > +
> > +                self.send_expect('echo 0000:%s >
> /sys/bus/pci/devices/0000\:%s\:%s/driver/unbind'
> 
> Here  if this devices has no driver binded, will failed, but seems no
> affect for logic, but at least should give some log message I think.
> 
> Thanks,
> Michael

Michael, here is just our logic. If there's no driver available for this nic, DTS will keep it untouched and load config for it. 
It's better to add some message for remind user DTS will not restore this nic. 

> > +                                 % (pci_bus, bus_id, devfun_id), '# ')
> > +                # bind to linux kernel driver
> > +                self.send_expect('modprobe %s' % driver, '# ')
> > +                self.send_expect('echo 0000:%s >
> /sys/bus/pci/drivers/%s/bind'
> > +                                 % (pci_bus, driver), '# ')
> > +                itf = self.get_interface_name(addr_array[0],
> addr_array[1])
> > +                self.send_expect("ifconfig %s up" % itf, "# ")
> > +
> > +    def load_portconf(self):
> > +        """
> > +        Load port configurations for ports_info. If manually configured
> infor
> > +        not same as auto scanned, still use infor in configuration file.
> > +        """
> > +        for port in self.ports_info:
> > +            pci_bus = port['pci']
> > +            if pci_bus in self.conf.ports_cfg:
> > +                port_cfg = self.conf.ports_cfg[pci_bus]
> > +                port_cfg['source'] = 'cfg'
> > +            else:
> > +                port_cfg = {}
> > +
> > +            if 'intf' in port_cfg:
> > +                if 'intf' in port:
> > +                    if port_cfg['intf'] != port['intf']:
> > +                        self.logger.warning("CONFIGURED INTERFACE NOT
> SAME AS SCANNED!!!")
> > +                port['intf'] = port_cfg['intf']
> > +
> > +            if 'mac' in port_cfg:
> > +                if 'mac' in port:
> > +                    if port_cfg['mac'] != port['mac']:
> > +                        self.logger.warning("CONFIGURED MACADDRESS NOT
> SAME AS SCANNED!!!")
> > +                port['mac'] = port_cfg['mac']
> > +
> > +            if 'numa' in port_cfg:
> > +                if 'numa' in port:
> > +                    if port_cfg['numa'] != port['numa']:
> > +                        self.logger.warning("CONFIGURED NUMA NOT SAME
> AS SCANNED!!!")
> > +                port['numa'] = port_cfg['numa']
> > +
> > +            if 'numa' in port_cfg:
> > +                port['peer'] = port_cfg['peer']
> 
> Why "if 'numa' in port_cfg:" twice here?
> 
> Also it is almost the same for those:
> 
> if key in port_cfg:
> 
> So can we do a iteration here?Like below:
> 
> for key in port_cfg:
>     if key in port:
>         if port_cfg['intf'] != port['intf']:
>             self.logger.warning("CONFIGURED INTERFACE NOT SAME AS
> SCANNED!!!")
>         port[key] = port_cfg[key]
> 
> 
> Thanks,
> Michael
> 

Thanks for this catch, the second "numa" should be "peer".
Will implement one function replace of these duplicate codes.
 
> > diff --git a/framework/project_dpdk.py b/framework/project_dpdk.py
> > index 6e25f1f..13be836 100644
> > --- a/framework/project_dpdk.py
> > +++ b/framework/project_dpdk.py
> > @@ -234,13 +234,10 @@ class DPDKdut(Dut):
> >          binding_list = '--bind=%s ' % driver
> >
> >          current_nic = 0
> > -        for (pci_bus, pci_id) in self.pci_devices_info:
> > -            if dts.accepted_nic(pci_id):
> > -
> > -                if nics_to_bind is None or current_nic in nics_to_bind:
> > -                    binding_list += '%s ' % (pci_bus)
> > -
> > -                current_nic += 1
> > +        for port_info in self.ports_info:
> > +            if nics_to_bind is None or current_nic in nics_to_bind:
> > +                binding_list += '%s ' % (port_info['pci'])
> > +            current_nic += 1
> >
> >          self.send_expect('tools/dpdk_nic_bind.py %s' % binding_list, '#
> ')
> >
> > @@ -252,13 +249,10 @@ class DPDKdut(Dut):
> >          binding_list = '-u '
> >
> >          current_nic = 0
> > -        for (pci_bus, pci_id) in self.pci_devices_info:
> > -            if dts.accepted_nic(pci_id):
> > -
> > -                if nics_to_bind is None or current_nic in nics_to_bind:
> > -                    binding_list += '%s ' % (pci_bus)
> > -
> > -                current_nic += 1
> > +        for port_info in self.ports_info:
> > +            if nics_to_bind is None or current_nic in nics_to_bind:
> > +                binding_list += '%s ' % (port_info['pci'])
> > +            current_nic += 1
> >
> >          self.send_expect('tools/dpdk_nic_bind.py %s' % binding_list, '#
> ', 30)
> >
> > diff --git a/framework/tester.py b/framework/tester.py
> > index 2c023dd..345ab41 100644
> > --- a/framework/tester.py
> > +++ b/framework/tester.py
> > @@ -170,6 +170,24 @@ class Tester(Crb):
> >          else:
> >              return 'down'
> >
> > +    def restore_interfaces(self):
> > +        """
> > +        Restore Linux interfaces.
> > +        """
> > +        self.send_expect("modprobe igb", "# ", 20)
> > +        self.send_expect("modprobe ixgbe", "# ", 20)
> > +        self.send_expect("modprobe e1000e", "# ", 20)
> > +        self.send_expect("modprobe e1000", "# ", 20)
> > +
> > +        try:
> > +            for (pci_bus, pci_id) in self.pci_devices_info:
> > +                addr_array = pci_bus.split(':')
> > +                itf = self.get_interface_name(addr_array[0],
> addr_array[1])
> > +                self.send_expect("ifconfig %s up" % itf, "# ")
> > +
> > +        except Exception as e:
> > +            self.logger.error("   !!! Restore ITF: " + e.message)
> > +
> >      def scan_ports(self):
> >          """
> >          Scan all ports on tester and save port's pci/mac/interface.
> > @@ -253,6 +271,18 @@ class Tester(Crb):
> >          hits = [False] * len(self.ports_info)
> >
> >          for dutPort in range(nrPorts):
> > +            peer = self.dut.get_peer_pci(dutPort)
> > +            if peer is not None:
> > +                for localPort in range(len(self.ports_info)):
> > +                    if self.ports_info[localPort]['pci'] == peer:
> > +                        hits[localPort] = True
> > +                        self.ports_map[dutPort] = localPort
> > +                        break
> > +                if self.ports_map[dutPort] == -1:
> > +                    self.logger.error("CONFIGURED TESTER PORT CANNOT
> FOUND!!!")
> > +                else:
> > +                    continue  # skip ping6 map
> > +
> >              for localPort in range(len(self.ports_info)):
> >                  if hits[localPort]:
> >                      continue