On Thu, Jun 11, 2015 at 9:31 AM, Jason Gunthorpe
<jguntho...@obsidianresearch.com> wrote:
> On Mon, Jun 08, 2015 at 05:12:13PM +0300, Matan Barak wrote:
>
>> +static struct net_device *mlx4_ib_get_netdev(struct ib_device *device, u8
>> port_num)
>> +{
>
> This function is never referenced in this patch, so we get compile
> warnings?
>
The motivation here was to add an implementation for the new RoCE GID
table management in one patch (this one) and replace it with the other
in the next patch. This way we have a working RoCE after each patch
while avoiding from having one big patch that adds the new
implementation and removes the old one.
The price is compilation warnings
> Warnings are not a huge deal, but you did compile test every patch in
> the series??
Yes we did and we are aware of the warnings.
>
>> + if (mlx4_is_bonded(ibdev->dev)) {
>> + struct net_device *dev;
>> + struct net_device *upper = NULL;
>
> So, I see this code in mlx4 touching bonding, but I don't see similar
> code in ocrdma..
>
> If bonding is a general feature why is this bit in the driver? Should
> the core code be doing this?
>
> Does bonding work in ocrdma at the end of this series? I was expecting
> it to..
>
This relates to feature in mlx4 called RoCE port aggregation. I guess
that this is not relevant to OCRDMA
Other bonding aspects (like populating addresses of an upper dev or
populating only on the port of the active slave) is being done in the
core the same way for all vendors
>> + gid_tbl = mailbox->buf;
>> +
>> + for (i = 0; i < MLX4_MAX_PORT_GIDS; ++i)
>> + memcpy(&gid_tbl[i], &gids[i].gid, sizeof(union ib_gid));
>> +
>> + err = mlx4_cmd(dev, mailbox->dma,
>> + MLX4_SET_PORT_GID_TABLE << 8 | port_num,
>> + 1, MLX4_CMD_SET_PORT, MLX4_CMD_TIME_CLASS_B,
>> + MLX4_CMD_WRAPPED);
>> + if (mlx4_is_bonded(dev))
>> + err += mlx4_cmd(dev, mailbox->dma,
>> + MLX4_SET_PORT_GID_TABLE << 8 | 2,
>> + 1, MLX4_CMD_SET_PORT, MLX4_CMD_TIME_CLASS_B,
>> + MLX4_CMD_WRAPPED);
>
> Again, wonder why the driver is sensitive to bonding, and ocrdma is not..
>
Same answer
>> @@ -477,11 +673,22 @@ out:
>> static int iboe_query_gid(struct ib_device *ibdev, u8 port, int index,
>> union ib_gid *gid)
>> {
>> - struct mlx4_ib_dev *dev = to_mdev(ibdev);
>> + int ret;
>>
>> - *gid = dev->iboe.gid_table[port - 1][index];
>> + if (!rdma_cap_roce_gid_table(ibdev, port)) {
>> + struct mlx4_ib_dev *dev = to_mdev(ibdev);
>>
>> - return 0;
>> + *gid = dev->iboe.gid_table[port - 1][index];
>> + return 0;
>> + }
>> +
>> + ret = ib_get_cached_gid(ibdev, port, index, gid);
>> + if (ret == -EAGAIN) {
>> + memcpy(gid, &zgid, sizeof(*gid));
>> + return 0;
>> + }
>> +
>> + return ret;
>> }
>
> Hum, is it Ok to change iboe_query_gid like this at this point in the
> series? Should this be in patch 11?
You are right. I'll fix.
>
> Jason
> --
> To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-rdma" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
