[U-Boot] [PATCH 07/11 v2] drivers/net/vsc9953: Add commands to manipulate the FDB for VSC9953

[Codrin Constantin Ciubotariu]

Hi Joe,

> -----Original Message-----
> From: Joe Hershberger [mailto:joe.hershberger at gmail.com]
> Sent: Wednesday, July 01, 2015 1:50 AM
> To: Ciubotariu Codrin Constantin-B43658
> Cc: u-boot; Joe Hershberger; Sun York-R58495
> Subject: Re: [U-Boot] [PATCH 07/11 v2] drivers/net/vsc9953: Add commands to
> manipulate the FDB for VSC9953
> 
> Hi Codrin,
> 
> On Tue, Jun 30, 2015 at 8:31 AM, Codrin Constantin Ciubotariu
> <codrin.ciubotariu at freescale.com> wrote:
> > Hi Joe,
> >
> >> -----Original Message-----
> >> From: Joe Hershberger [mailto:joe.hershberger at gmail.com]
> >> Sent: Friday, June 26, 2015 1:39 AM
> >> To: Ciubotariu Codrin Constantin-B43658
> >> Cc: u-boot; Joe Hershberger; Sun York-R58495
> >> Subject: Re: [U-Boot] [PATCH 07/11 v2] drivers/net/vsc9953: Add commands to
> >> manipulate the FDB for VSC9953
> >>
> >> > +       return !!timeout;
> >>
> >> Maybe return -EBUSY like suggested in previous patch.
> >
> > Ok.
> >
> >> > +       /* write port and vid to get selected FDB entries */
> >> > +       val = in_le32(&l2ana_reg->ana.anag_efil);
> >> > +       if (port_no != VSC9953_CMD_PORT_ALL) {
> >> > +               val = (val & ~CONFIG_VSC9953_AGE_PORT_MASK) |
> >> > +                               CONFIG_VSC9953_AGE_PORT_EN |
> >> > +                               field_set(port_no,
> >> > +                                         CONFIG_VSC9953_AGE_PORT_MASK);
> >>
> >> Seems like a good place to use bitfield_replace() from
> >> include/bitfield.h (or a new one that you add that uses the mask for
> >> the shift).
> >>
> >> > +       }
> >> > +       if (vid != VSC9953_CMD_VLAN_ALL) {
> >> > +               val = (val & ~CONFIG_VSC9953_AGE_VID_MASK) |
> >> > +                               CONFIG_VSC9953_AGE_VID_EN |
> >> > +                               field_set(vid,
> CONFIG_VSC9953_AGE_VID_MASK);
> >>
> >> Same here.
> >
> > Ok.
> >
> >> > +               vlan = field_get(val & CONFIG_VSC9953_MAC_VID_MASK,
> >> > +                                CONFIG_VSC9953_MAC_VID_MASK);
> >>
> >> It seems like masking off the val before shifting it would be better
> >> implemented inside the field_get function (renamed and moved to
> >> include/bitfield.h) instead of on each use.
> >
> > Yes, something like #define field_set(val, mask)           (((val) * ((mask) &
> ~((mask) << 1))) & mask) and
> > #define field_get(val, mask)           ((val & mask) / ((mask) & ~((mask) <<
> 1))).
> 
> The set seems the same as replace, just without any other bitfields to
> preserve. You could just use replace and pass in 0 as the reg_val.

Yes, although I still have to pass the "shift" parameter.

> 
> >> > +       out_le32(&l2ana_reg->ana_tables.mach_data,
> >> > +                (mac[0] << 8) | (mac[1] << 0) |
> >> > +                (field_set(vid, CONFIG_VSC9953_MACHDATA_VID_MASK) &
> >> > +                                CONFIG_VSC9953_MACHDATA_VID_MASK));
> >>
> >> Why do you need to & with the mask again after field_set()?
> >
> > To assure that the shifted vid value is not higher than its mask. Adding the
> mask to the macro/inline function as described above should assure this.
> 
> OK. Maybe the existing bitfield_replace() should be updated to also
> mask off the bitfield_val.

Yes, I think it should be updated. Should I make a separate patch for it? It should be out of this patch set since it has nothing to do with VSC9953.

> 
> >> > +       /* check if MAC address is present */
> >> > +       if (!parsed_cmd->mac_addr) {
> >>
> >> Use this:
> >> +       if (is_valid_ethaddr(parsed_cmd->mac_addr)) {
> >
> > is_valid_ethaddr() returns false if the mac address is 00:00:00:00:00:00, but
> for the L2 Switch, this mac address is valid. Maybe is_broadcast_ethaddr()?
> 
> I'm not sure what criteria you intend to check for here, so I'm not
> sure if that is appropriate.

When a command for adding/removing a mac address is issued, we should check if there is a valid mac address in parsed_cmd->mac_addr. I guess that this could be checked only in keyword_match_mac_addr(). Also, since mac_addr will become a static array, I should initialize this array with an invalid mac address. Since 0 mac address is a valid address for the L2 Switch, I was thinking to initialize it with L2 Broadcast address. What do you think?

> 
> >> > +       /* check if MAC address is present */
> >> > +       if (!parsed_cmd->mac_addr) {
> >>
> >> Use this:
> >> +       if (is_valid_ethaddr(parsed_cmd->mac_addr)) {
> >
> > Same as above.
> >
> >> > +/* check if the string has the format for a MAC address */
> >> > +static int string_is_mac_addr(const char *mac)
> >> > +{
> >> > +       int                     i;
> >> > +
> >> > +       if (!mac)
> >> > +               return 0;
> >> > +
> >> > +       for (i = 0; i < 6; i++) {
> >> > +               if (!isxdigit(*mac) || !isxdigit(*(mac + 1)))
> >> > +                       return 0;
> >> > +               mac += 2;
> >> > +               if (i != 5) {
> >> > +                       if (*mac != ':' && *mac != '-')
> >> > +                               return 0;
> >> > +                       mac++;
> >> > +               }
> >> > +       }
> >> > +
> >> > +       if (*mac != '\0')
> >> > +               return 0;
> >> > +
> >> > +       return 1;
> >>
> >> This functionality is already implemented in common/env_flags.c in the
> >> _env_flags_validate_type() function. Maybe that implementation should
> >> be extracted.  Another option is to make the eth_parse_enetaddr() in
> >> net/eth.c return a status and then it can be used. Either way I don't
> >> think it should be re-implemented here.
> >
> > Yes, I noticed that this function is already implemented, but with no access
> to it. I will see how I can use the one available.
> 
> Which path do you plan to take?

I looked through both eth_parse_enetaddr() and _env_flags_validate_type() and I prefer the implementation from the latter function, since eth_parse_enetaddr() doesn't check for ':' or if the value returned by simple_strtoul() is <= 0xFF. I could move the code from "case env_flags_vartype_macaddr:" to a separate function (something like eth_check_enetaddr()? ) and add its prototype somewhere in include/net.h. What do you think?

Best regards,
Codrin