Re: [PATCH net-next, 5/6] net/mlx5: Add HV VHCA control agent

[Eran Ben Elisha <eranbe@mellanox.com>]

On 8/14/2019 11:41 PM, Mark Bloch wrote:
> 
> 
> On 8/14/19 12:09 PM, Haiyang Zhang wrote:
>> From: Eran Ben Elisha <eranbe@mellanox.com>
>>
>> Control agent is responsible over of the control block (ID 0). It should
>> update the PF via this block about every capability change. In addition,
>> upon block 0 invalidate, it should activate all other supported agents
>> with data requests from the PF.
>>
>> Upon agent create/destroy, the invalidate callback of the control agent
>> is being called in order to update the PF driver about this change.
>>
>> The control agent is an integral part of HV VHCA and will be created
>> and destroy as part of the HV VHCA init/cleanup flow.
>>
>> Signed-off-by: Eran Ben Elisha <eranbe@mellanox.com>
>> Signed-off-by: Saeed Mahameed <saeedm@mellanox.com>
>> ---
>>   .../net/ethernet/mellanox/mlx5/core/lib/hv_vhca.c  | 122 ++++++++++++++++++++-
>>   .../net/ethernet/mellanox/mlx5/core/lib/hv_vhca.h  |   1 +
>>   2 files changed, 121 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/hv_vhca.c b/drivers/net/ethernet/mellanox/mlx5/core/lib/hv_vhca.c
>> index b2eebdf..3c7fffa 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/hv_vhca.c
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/hv_vhca.c
>> @@ -110,22 +110,131 @@ void mlx5_hv_vhca_invalidate(void *context, u64 block_mask)
>>   	queue_work(hv_vhca->work_queue, &work->invalidate_work);
>>   }
>>   
>> +#define AGENT_MASK(type) (type ? BIT(type - 1) : 0 /* control */)
>> +
>> +static void mlx5_hv_vhca_agents_control(struct mlx5_hv_vhca *hv_vhca,
>> +					struct mlx5_hv_vhca_control_block *block)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < MLX5_HV_VHCA_AGENT_MAX; i++) {
>> +		struct mlx5_hv_vhca_agent *agent = hv_vhca->agents[i];
>> +
>> +		if (!agent || !agent->control)
>> +			continue;
>> +
>> +		if (!(AGENT_MASK(agent->type) & block->control))
>> +			continue;
>> +
>> +		agent->control(agent, block);
>> +	}
>> +}
>> +
>> +static void mlx5_hv_vhca_capabilities(struct mlx5_hv_vhca *hv_vhca,
>> +				      u32 *capabilities)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < MLX5_HV_VHCA_AGENT_MAX; i++) {
>> +		struct mlx5_hv_vhca_agent *agent = hv_vhca->agents[i];
>> +
>> +		if (agent)
>> +			*capabilities |= AGENT_MASK(agent->type);
>> +	}
>> +}
>> +
>> +static void
>> +mlx5_hv_vhca_control_agent_invalidate(struct mlx5_hv_vhca_agent *agent,
>> +				      u64 block_mask)
>> +{
>> +	struct mlx5_hv_vhca *hv_vhca = agent->hv_vhca;
>> +	struct mlx5_core_dev *dev = hv_vhca->dev;
>> +	struct mlx5_hv_vhca_control_block *block;
>> +	u32 capabilities = 0;
>> +	int err;
>> +
>> +	block = kzalloc(sizeof(*block), GFP_KERNEL);
>> +	if (!block)
>> +		return;
>> +
>> +	err = mlx5_hv_read_config(dev, block, sizeof(*block), 0);
>> +	if (err)
>> +		goto free_block;
>> +
>> +	mlx5_hv_vhca_capabilities(hv_vhca, &capabilities);
>> +
>> +	/* In case no capabilities, send empty block in return */
>> +	if (!capabilities) {
>> +		memset(block, 0, sizeof(*block));
>> +		goto write;
>> +	}
>> +
>> +	if (block->capabilities != capabilities)
>> +		block->capabilities = capabilities;
>> +
>> +	if (block->control & ~capabilities)
>> +		goto free_block;
>> +
>> +	mlx5_hv_vhca_agents_control(hv_vhca, block);
>> +	block->command_ack = block->command;
>> +
>> +write:
>> +	mlx5_hv_write_config(dev, block, sizeof(*block), 0);
>> +
>> +free_block:
>> +	kfree(block);
>> +}
>> +
>> +static struct mlx5_hv_vhca_agent *
>> +mlx5_hv_vhca_control_agent_create(struct mlx5_hv_vhca *hv_vhca)
>> +{
>> +	return mlx5_hv_vhca_agent_create(hv_vhca, MLX5_HV_VHCA_AGENT_CONTROL,
>> +					 NULL,
>> +					 mlx5_hv_vhca_control_agent_invalidate,
>> +					 NULL, NULL);
>> +}
>> +
>> +static void mlx5_hv_vhca_control_agent_destroy(struct mlx5_hv_vhca_agent *agent)
>> +{
>> +	mlx5_hv_vhca_agent_destroy(agent);
>> +}
>> +
>>   int mlx5_hv_vhca_init(struct mlx5_hv_vhca *hv_vhca)
>>   {
>> +	struct mlx5_hv_vhca_agent *agent;
>> +	int err;
>> +
>>   	if (IS_ERR_OR_NULL(hv_vhca))
>>   		return IS_ERR_OR_NULL(hv_vhca);
>>   
>> -	return mlx5_hv_register_invalidate(hv_vhca->dev, hv_vhca,
>> -					   mlx5_hv_vhca_invalidate);
>> +	err = mlx5_hv_register_invalidate(hv_vhca->dev, hv_vhca,
>> +					  mlx5_hv_vhca_invalidate);
>> +	if (err)
>> +		return err;
>> +
>> +	agent = mlx5_hv_vhca_control_agent_create(hv_vhca);
>> +	if (IS_ERR_OR_NULL(agent)) {
>> +		mlx5_hv_unregister_invalidate(hv_vhca->dev);
>> +		return IS_ERR_OR_NULL(agent);
>> +	}
>> +
>> +	hv_vhca->agents[MLX5_HV_VHCA_AGENT_CONTROL] = agent;
>> +
>> +	return 0;
>>   }
>>   
>>   void mlx5_hv_vhca_cleanup(struct mlx5_hv_vhca *hv_vhca)
>>   {
>> +	struct mlx5_hv_vhca_agent *agent;
>>   	int i;
>>   
>>   	if (IS_ERR_OR_NULL(hv_vhca))
>>   		return;
>>   
>> +	agent = hv_vhca->agents[MLX5_HV_VHCA_AGENT_CONTROL];
>> +	if (!IS_ERR_OR_NULL(agent))
>> +		mlx5_hv_vhca_control_agent_destroy(agent);
> 
> Can the agent be err ptr here?

Only NULL, will fix.

> 
>> +
>>   	mutex_lock(&hv_vhca->agents_lock);
>>   	for (i = 0; i < MLX5_HV_VHCA_AGENT_MAX; i++)
>>   		WARN_ON(hv_vhca->agents[i]);
> 
> With the comment above in mind, here you check only for not null

Comment above was right... after fixing it, all is aligned here.

> 
>> @@ -135,6 +244,11 @@ void mlx5_hv_vhca_cleanup(struct mlx5_hv_vhca *hv_vhca)
>>   	mlx5_hv_unregister_invalidate(hv_vhca->dev);
>>   }
>>   
>> +static void mlx5_hv_vhca_agents_update(struct mlx5_hv_vhca *hv_vhca)
>> +{
>> +	mlx5_hv_vhca_invalidate(hv_vhca, BIT(MLX5_HV_VHCA_AGENT_CONTROL));
>> +}
>> +
>>   struct mlx5_hv_vhca_agent *
>>   mlx5_hv_vhca_agent_create(struct mlx5_hv_vhca *hv_vhca,
>>   			  enum mlx5_hv_vhca_agent_type type,
>> @@ -168,6 +282,8 @@ struct mlx5_hv_vhca_agent *
>>   	hv_vhca->agents[type] = agent;
>>   	mutex_unlock(&hv_vhca->agents_lock);
>>   
>> +	mlx5_hv_vhca_agents_update(hv_vhca);
>> +
>>   	return agent;
>>   }
>>   
>> @@ -189,6 +305,8 @@ void mlx5_hv_vhca_agent_destroy(struct mlx5_hv_vhca_agent *agent)
>>   		agent->cleanup(agent);
>>   
>>   	kfree(agent);
>> +
>> +	mlx5_hv_vhca_agents_update(hv_vhca);
>>   }
>>   
>>   static int mlx5_hv_vhca_data_block_prepare(struct mlx5_hv_vhca_agent *agent,
>> diff --git a/drivers/net/ethernet/mellanox/mlx5/core/lib/hv_vhca.h b/drivers/net/ethernet/mellanox/mlx5/core/lib/hv_vhca.h
>> index fa7ee85..6f4bfb1 100644
>> --- a/drivers/net/ethernet/mellanox/mlx5/core/lib/hv_vhca.h
>> +++ b/drivers/net/ethernet/mellanox/mlx5/core/lib/hv_vhca.h
>> @@ -12,6 +12,7 @@
>>   struct mlx5_hv_vhca_control_block;
>>   
>>   enum mlx5_hv_vhca_agent_type {
>> +	MLX5_HV_VHCA_AGENT_CONTROL = 0,
> 
> No need to start value

I find it more easy to read when having the value explicitly.
If you or Saeed has a strong opinion against it, this can be easily fixed.

> 
>>   	MLX5_HV_VHCA_AGENT_MAX = 32,
>>   };
>>   
>>
> 
> Mark
>