Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Message-ID: <b268bd69-7d2b-6236-b5cc-1f8058165d31@intel.com>
Date:   Wed, 28 Sep 2022 08:38:26 -0700
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (Windows NT 10.0; Win64; x64; rv:102.0) Gecko/20100101
 Firefox/102.0 Thunderbird/102.3.0
Subject: Re: [PATCH 2/2] dmaengine: idxd: track enabled workqueues in bitmap
Content-Language: en-US
To:     Jerry Snitselaar <jsnitsel@redhat.com>,
        linux-kernel@vger.kernel.org, dmaengine@vger.kernel.org
Cc:     Fenghua Yu <fenghua.yu@intel.com>, Vinod Koul <vkoul@kernel.org>
References: <20220919215553.600246-1-jsnitsel@redhat.com>
 <20220919215553.600246-3-jsnitsel@redhat.com>
 <20220928152120.3wsvc4iungzsmryn@cantor>
From:   Dave Jiang <dave.jiang@intel.com>
In-Reply-To: <20220928152120.3wsvc4iungzsmryn@cantor>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: 7bit
Precedence: bulk


On 9/28/2022 8:21 AM, Jerry Snitselaar wrote:
> On Mon, Sep 19, 2022 at 02:55:53PM -0700, Jerry Snitselaar wrote:
>> Now that idxd_wq_disable_cleanup() sets the workqueue state to
>> IDXD_WQ_DISABLED, use a bitmap to track which workqueues have been
>> enabled. This will then be used to determine which workqueues
>> should be re-enabled when attempting a software reset to recover
>> from a device halt state.
>>
>> Cc: Fenghua Yu <fenghua.yu@intel.com>
>> Cc: Dave Jiang <dave.jiang@intel.com>
>> Cc: Vinod Koul <vkoul@kernel.org>
>> Signed-off-by: Jerry Snitselaar <jsnitsel@redhat.com>
>> ---
>>   drivers/dma/idxd/device.c | 2 ++
>>   drivers/dma/idxd/idxd.h   | 2 ++
>>   drivers/dma/idxd/init.c   | 6 ++++++
>>   drivers/dma/idxd/irq.c    | 4 ++--
>>   drivers/dma/idxd/sysfs.c  | 1 +
>>   5 files changed, 13 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
>> index 31911e255ac1..f0c7d6d348e3 100644
>> --- a/drivers/dma/idxd/device.c
>> +++ b/drivers/dma/idxd/device.c
>> @@ -196,6 +196,7 @@ int idxd_wq_enable(struct idxd_wq *wq)
>>   	}
>>   
>>   	wq->state = IDXD_WQ_ENABLED;
>> +	set_bit(wq->id, idxd->wq_enable_map);
>>   	dev_dbg(dev, "WQ %d enabled\n", wq->id);
>>   	return 0;
>>   }
>> @@ -223,6 +224,7 @@ int idxd_wq_disable(struct idxd_wq *wq, bool reset_config)
>>   
>>   	if (reset_config)
>>   		idxd_wq_disable_cleanup(wq);
>> +	clear_bit(wq->id, idxd->wq_enable_map);
>>   	wq->state = IDXD_WQ_DISABLED;
>>   	dev_dbg(dev, "WQ %d disabled\n", wq->id);
>>   	return 0;
>> diff --git a/drivers/dma/idxd/idxd.h b/drivers/dma/idxd/idxd.h
>> index fed0dfc1eaa8..f527a7f88b92 100644
>> --- a/drivers/dma/idxd/idxd.h
>> +++ b/drivers/dma/idxd/idxd.h
>> @@ -11,6 +11,7 @@
>>   #include <linux/idr.h>
>>   #include <linux/pci.h>
>>   #include <linux/ioasid.h>
>> +#include <linux/bitmap.h>
>>   #include <linux/perf_event.h>
>>   #include <uapi/linux/idxd.h>
>>   #include "registers.h"
>> @@ -299,6 +300,7 @@ struct idxd_device {
>>   	int rdbuf_limit;
>>   	int nr_rdbufs;		/* non-reserved read buffers */
>>   	unsigned int wqcfg_size;
>> +	unsigned long *wq_enable_map;
>>   
>>   	union sw_err_reg sw_err;
>>   	wait_queue_head_t cmd_waitq;
>> diff --git a/drivers/dma/idxd/init.c b/drivers/dma/idxd/init.c
>> index aa3478257ddb..7e27e69ff741 100644
>> --- a/drivers/dma/idxd/init.c
>> +++ b/drivers/dma/idxd/init.c
>> @@ -151,6 +151,12 @@ static int idxd_setup_wqs(struct idxd_device *idxd)
>>   	if (!idxd->wqs)
>>   		return -ENOMEM;
>>   
>> +	idxd->wq_enable_map = bitmap_zalloc_node(idxd->max_wqs, GFP_KERNEL, dev_to_node(dev));
>> +	if (!idxd->wq_enable_map) {
>> +		kfree(idxd->wqs);
>> +		return -ENOMEM;
>> +	}
>> +
>>   	for (i = 0; i < idxd->max_wqs; i++) {
>>   		wq = kzalloc_node(sizeof(*wq), GFP_KERNEL, dev_to_node(dev));
>>   		if (!wq) {
>> diff --git a/drivers/dma/idxd/irq.c b/drivers/dma/idxd/irq.c
>> index 743ead5ebc57..8efaf137fc65 100644
>> --- a/drivers/dma/idxd/irq.c
>> +++ b/drivers/dma/idxd/irq.c
>> @@ -49,9 +49,9 @@ static void idxd_device_reinit(struct work_struct *work)
>>   		goto out;
>>   
>>   	for (i = 0; i < idxd->max_wqs; i++) {
>> -		struct idxd_wq *wq = idxd->wqs[i];
>> +		if (test_bit(i, idxd->wq_enable_map)) {
>> +			struct idxd_wq *wq = idxd->wqs[i];
>>   
>> -		if (wq->state == IDXD_WQ_ENABLED) {
>>   			rc = idxd_wq_enable(wq);
>>   			if (rc < 0) {
>>   				dev_warn(dev, "Unable to re-enable wq %s\n",
> Hi Dave and Fenghua,
>
> Thinking about this last night, this should probably clear the bit here in
> the case where an error is returned from idxd_wq_enable here, yes? I can
> send a v2.
Ah yeah I think so. We failed to enable, so just give up. Thanks.
>
>> diff --git a/drivers/dma/idxd/sysfs.c b/drivers/dma/idxd/sysfs.c
>> index 3f262a57441b..3325b16ed959 100644
>> --- a/drivers/dma/idxd/sysfs.c
>> +++ b/drivers/dma/idxd/sysfs.c
>> @@ -1405,6 +1405,7 @@ static void idxd_conf_device_release(struct device *dev)
>>   	struct idxd_device *idxd = confdev_to_idxd(dev);
>>   
>>   	kfree(idxd->groups);
>> +	bitmap_free(idxd->wq_enable_map);
>>   	kfree(idxd->wqs);
>>   	kfree(idxd->engines);
>>   	ida_free(&idxd_ida, idxd->id);
>> -- 
>> 2.37.2
>>