On 08/07/2018 12:19 PM, Leon Romanovsky wrote:
> On Tue, Aug 07, 2018 at 10:53:13AM +0200, Ursula Braun wrote:
>> The mlx4 driver does not trigger an IB_EVENT_PORT_ACTIVE when the
>> RoCE network interface is activated. When SMC determines the RoCE
>> device port to be used, it checks the port states.
>> This patch triggers IB events for NETDEV_UP and NETDEV_DOWN.
>>
>> Signed-off-by: Ursula Braun <ubraun@xxxxxxxxxxxxx>
>> ---
>> drivers/infiniband/hw/mlx4/main.c | 20 ++++++++++++++++++++
>> 1 file changed, 20 insertions(+)
>>
>> diff --git a/drivers/infiniband/hw/mlx4/main.c b/drivers/infiniband/hw/mlx4/main.c
>> index 4ec519afc45b..81a1fd9e8615 100644
>> --- a/drivers/infiniband/hw/mlx4/main.c
>> +++ b/drivers/infiniband/hw/mlx4/main.c
>> @@ -2447,6 +2447,26 @@ static void mlx4_ib_scan_netdevs(struct mlx4_ib_dev *ibdev,
>> event == NETDEV_UP || event == NETDEV_CHANGE))
>> update_qps_port = port;
>>
>> + if (dev == iboe->netdevs[port - 1] &&
>> + event == NETDEV_UP) {
>> + struct ib_event ibev = { };
>> +
>> + ibev.device = &ibdev->ib_dev;
>> + ibev.element.port_num = port;
>> + ibev.event = IB_EVENT_PORT_ACTIVE;
>> + ib_dispatch_event(&ibev);
>> + }
>> +
>> + if (dev == iboe->netdevs[port - 1] &&
>> + event == NETDEV_DOWN) {
>> + struct ib_event ibev = { };
>> +
>> + ibev.device = &ibdev->ib_dev;
>> + ibev.element.port_num = port;
>> + ibev.event = IB_EVENT_PORT_ERR;
>> + ib_dispatch_event(&ibev);
>> + }
>> +
>
> Thanks Ursula,
>
> I think that you need to take into account previous port states
> (IB_PORT_DOWN vs. IB_PORT_ACTIVE).
>
Do you want to skip raising an IB event in these scenarios:
NETDEV_UP event and previous port state is already IB_PORT_ACTIVE
NETDEV_UP event and current port state is IB_PORT_DOWN
NETDEV_DOWN event and previous port state is IB_PORT_DOWN
NETDEV_DOWN event and current port state is IB_PORT_UP
Isn't it guaranteed that a NETDEV_UP event is seen only when the port state has
been IB_PORT_DOWN before (and vice versa)? At least the net/core/dev.c code makes
sure, a NETDEV_UP event is not raised twice.
And even if the previous port state could be the same, would it hurt if an IB_PORT_ACTIVE
or IB_PORT_DOWN event is raised a second time in rare cases?
Nevertheless I tried to determine the current port state, and found:
If I use ib_get_cached_port_state(), I still get IB_PORT_DOWN for the NETDEV_UP event
and vice versa. If I call ibdev->query_port() I run into locking problems, since the
iboe->lock is held.
For the net/smc code the main problem is the missing IB event in the mlx4 driver, since
the net/smc code relies on it. If there are scenarios raising the same IB event more than
once, this does not cause trouble.
Taking into account the previous port state might be an optimization, but facing the
problems described I am not sure if it is really worth spending extra code to maintain
the previous and current port state.
Do you have a sample demonstrating the urgent need for that?
> Thanks
>
Attachment:
signature.asc
Description: OpenPGP digital signature
