Hi, Sorry about the delay. > -----Original Message----- > From: Jason Gunthorpe [mailto:jgunthorpe@xxxxxxxxxxxxxxxxxxxx] > Sent: Wednesday, May 04, 2016 9:41 PM > To: Leon Romanovsky <leon@xxxxxxxxxx> > Cc: dledford@xxxxxxxxxx; linux-rdma@xxxxxxxxxxxxxxx; Mark Bloch > <markb@xxxxxxxxxxxx>; Majd Dibbiny <majd@xxxxxxxxxxxx>; Matan > Barak <matanb@xxxxxxxxxxxx> > Subject: Re: [PATCH rdma-next] IB/IPoIB: Allow setting the device address > > On Wed, May 04, 2016 at 08:36:47AM +0300, Leon Romanovsky wrote: > > +static int ipoib_set_mac(struct net_device *dev, void *addr) > > +{ > > + struct ipoib_dev_priv *priv = netdev_priv(dev); > > + struct sockaddr_storage *ss = addr; > > + > > + set_base_guid(priv, (union ib_gid *)(ss->__data + 4), 0); > > Hurm, this should also check that the QPN and reserved in the provided > 20 byte LLADDR match the current state of the device's LLADDR since > those cannot be changed. > > It is should also be illegal to try and change the GID prefix, so it > must also match the current value. I didn't think we need those checks as we are taking only the GUID part, but I'll add them. Better safe than sorry I guess :) > Did these checks from eth_prepare_mac_addr_change get included? > > if (!(dev->priv_flags & IFF_LIVE_ADDR_CHANGE) && > netif_running(dev)) > return -EBUSY; > Why do I need to include that? > As well as basic sanity checks on the formation of GUID - can't be 0, > and must be unicast. I'll add the GUID != 0 check. The unicast is covered under a previous check, making sure the first 12 bytes are the same. If dev_addr[4] != 0xff -> not multicast, and we start with a none multicast lladdr. > > + if (test_bit(IPOIB_FLAG_DEV_ADDR_SET, &priv->flags)) { > > + priv->local_gid.global.subnet_prefix = > > + cpu_to_be64(port_attr.subnet_prefix); > > + memcpy(dev->dev_addr + 4, priv->local_gid.raw, > > + sizeof(union ib_gid)); > > Seems strange - why is the subnet prefix being treated differently in > this one case? Just wanted to make sure we are not trying to join a multicast group with the wrong value. I'll move the copy to: ipoib_dev_addr_changed_valid, this way when a subnet prefix changes It will be copied both to local_gid and dev_addr. > Jason -- To unsubscribe from this list: send the line "unsubscribe linux-rdma" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
