Linux-HyperV Archive on lore.kernel.org
 help / color / Atom feed
From: Eran Ben Elisha <eranbe@mellanox.com>
To: Mark Bloch <markb@mellanox.com>,
	haiyangz <haiyangz@microsoft.com>,
	"sashal@kernel.org" <sashal@kernel.org>,
	"davem@davemloft.net" <davem@davemloft.net>,
	Saeed Mahameed <saeedm@mellanox.com>,
	"leon@kernel.org" <leon@kernel.org>,
	"lorenzo.pieralisi@arm.com" <lorenzo.pieralisi@arm.com>,
	"bhelgaas@google.com" <bhelgaas@google.com>,
	"linux-pci@vger.kernel.org" <linux-pci@vger.kernel.org>,
	"linux-hyperv@vger.kernel.org" <linux-hyperv@vger.kernel.org>,
	"netdev@vger.kernel.org" <netdev@vger.kernel.org>
Cc: kys <kys@microsoft.com>,
	Stephen Hemminger <sthemmin@microsoft.com>,
	"linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>
Subject: Re: [PATCH net-next, 5/6] net/mlx5: Add HV VHCA control agent
Date: Thu, 15 Aug 2019 11:35:36 +0000
Message-ID: <f5f65f2d-41cb-30a8-d20b-605093bc7c0d@mellanox.com> (raw)
In-Reply-To: <745f663e-0c56-84d0-a02b-106f788e3e8f@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
> 

  reply index

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-14 19:08 [PATCH net-next, 0/6] Add software backchannel and mlx5e HV VHCA stats Haiyang Zhang
2019-08-14 19:08 ` [PATCH net-next, 1/6] PCI: hv: Add a paravirtual backchannel in software Haiyang Zhang
2019-08-14 19:08 ` [PATCH net-next, 2/6] PCI: hv: Add a Hyper-V PCI mini driver for software backchannel interface Haiyang Zhang
2019-08-16 12:27   ` Vitaly Kuznetsov
2019-08-16 14:48     ` Haiyang Zhang
2019-08-16 16:16       ` Vitaly Kuznetsov
2019-08-16 19:50         ` Haiyang Zhang
2019-08-14 19:08 ` [PATCH net-next, 3/6] net/mlx5: Add wrappers for HyperV PCIe operations Haiyang Zhang
2019-08-14 20:41   ` Mark Bloch
2019-08-15 11:34     ` Eran Ben Elisha
2019-08-19 15:04       ` Haiyang Zhang
2019-08-14 19:08 ` [PATCH net-next, 4/6] net/mlx5: Add HV VHCA infrastructure Haiyang Zhang
2019-08-14 20:41   ` Mark Bloch
2019-08-15 11:35     ` Eran Ben Elisha
2019-08-14 19:09 ` [PATCH net-next, 5/6] net/mlx5: Add HV VHCA control agent Haiyang Zhang
2019-08-14 20:41   ` Mark Bloch
2019-08-15 11:35     ` Eran Ben Elisha [this message]
2019-08-14 19:09 ` [PATCH net-next, 6/6] net/mlx5e: Add mlx5e HV VHCA stats agent Haiyang Zhang

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f5f65f2d-41cb-30a8-d20b-605093bc7c0d@mellanox.com \
    --to=eranbe@mellanox.com \
    --cc=bhelgaas@google.com \
    --cc=davem@davemloft.net \
    --cc=haiyangz@microsoft.com \
    --cc=kys@microsoft.com \
    --cc=leon@kernel.org \
    --cc=linux-hyperv@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-pci@vger.kernel.org \
    --cc=lorenzo.pieralisi@arm.com \
    --cc=markb@mellanox.com \
    --cc=netdev@vger.kernel.org \
    --cc=saeedm@mellanox.com \
    --cc=sashal@kernel.org \
    --cc=sthemmin@microsoft.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-HyperV Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-hyperv/0 linux-hyperv/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-hyperv linux-hyperv/ https://lore.kernel.org/linux-hyperv \
		linux-hyperv@vger.kernel.org
	public-inbox-index linux-hyperv

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-hyperv


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git