Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
To:     Keith Busch <kbusch@kernel.org>
Cc:     linux-kernel@vger.kernel.org, linux-nvme@lists.infradead.org,
        linux-block@vger.kernel.org, Christoph Hellwig <hch@lst.de>,
        Sagi Grimberg <sagi@grimberg.me>,
        Keith Busch <keith.busch@wdc.com>, Jens Axboe <axboe@kernel.dk>
References: <20200812210119.7155-1-logang@deltatee.com>
 <20200812211959.GA1238450@dhcp-10-100-145-180.wdl.wdc.com>
From:   Logan Gunthorpe <logang@deltatee.com>
Message-ID: <b86772c1-17ed-ec44-1b60-7f2ad80bd73c@deltatee.com>
Date:   Wed, 12 Aug 2020 15:31:41 -0600
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:68.0) Gecko/20100101
 Thunderbird/68.11.0
MIME-Version: 1.0
In-Reply-To: <20200812211959.GA1238450@dhcp-10-100-145-180.wdl.wdc.com>
Content-Type: text/plain; charset=utf-8
Content-Language: en-US
Content-Transfer-Encoding: 7bit
Subject: Re: [RESEND PATCH] nvme: Use spin_lock_irqsave() when taking the
 ctrl->lock
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk


On 2020-08-12 3:19 p.m., Keith Busch wrote:
> On Wed, Aug 12, 2020 at 03:01:19PM -0600, Logan Gunthorpe wrote:
>> @@ -2971,15 +2971,16 @@ int nvme_get_log(struct nvme_ctrl *ctrl, u32 nsid, u8 log_page, u8 lsp, u8 csi,
>>  static struct nvme_cel *nvme_find_cel(struct nvme_ctrl *ctrl, u8 csi)
>>  {
>>  	struct nvme_cel *cel, *ret = NULL;
>> +	unsigned long flags;
>>
>> -	spin_lock(&ctrl->lock);
>> +	spin_lock_irqsave(&ctrl->lock, flags);
>>  	list_for_each_entry(cel, &ctrl->cels, entry) {
>>  		if (cel->csi == csi) {
>>  			ret = cel;
>>  			break;
>>  		}
>>  	}
>> -	spin_unlock(&ctrl->lock);
>> +	spin_unlock_irqrestore(&ctrl->lock, flags);
>>
>>  	return ret;
>>  }
>> @@ -2988,6 +2989,7 @@ static int nvme_get_effects_log(struct nvme_ctrl *ctrl, u8 csi,
>>  				struct nvme_effects_log **log)
>>  {
>>  	struct nvme_cel *cel = nvme_find_cel(ctrl, csi);
>> +	unsigned long flags;
>>  	int ret;
>>
>>  	if (cel)
>> @@ -3006,9 +3008,9 @@ static int nvme_get_effects_log(struct nvme_ctrl *ctrl, u8 csi,
>>
>>  	cel->csi = csi;
>>
>> -	spin_lock(&ctrl->lock);
>> +	spin_lock_irqsave(&ctrl->lock, flags);
>>  	list_add_tail(&cel->entry, &ctrl->cels);
>> -	spin_unlock(&ctrl->lock);
>> +	spin_unlock_irqrestore(&ctrl->lock, flags);
>>  out:
>>  	*log = &cel->log;
>>  	return 0;
>>
> 
> Neither of these are ever called from an interrupt disabled context,
> correct? If so, you can just use spin_lock_irq() without saving the
> current irq state.

Yes, I believe that is correct. I'll send a v2 shortly.

Thanks,

Logan