[PATCH v2 0/3] block/nvme: Fix DMA-noncoherent platforms support

[PATCH v2 0/3] block/nvme: Fix DMA-noncoherent platforms support

[Serge Semin]

Our SoC doesn't have the CPU caches coherent on DMA's. After getting the
kernel updated to the 6.0-rcX version we've discovered a problem with the
NVME hwmon probe. It turned out that the root cause of it was connected
with the cache-line-unaligned buffer passed to the DMA-engine. Due to the
cache-invalidation performed on the buffer mapping stage a part of the
structure the buffer was embedded to was lost. Here we suggest to fix the
problem just by aligning the buffer accordingly as the
Documentation/core-api/dma-api.rst document requires. (See the
corresponding patch log for more details.)

A potential root of a similar problem has been detected in the sed-opal
driver too. Even though we have not got any difficulties connected with
that part we still suggest to fix that in the same way as it is done for
the NVME hwmon driver.

Link: https://lore.kernel.org/linux-nvme/20220909191916.16013-1-Sergey.Semin@baikalelectronics.ru
Changelog v2:
- Drop Jonathan Derrick and Revanth Rajashekar from Cc-list due to
  the email messages being bounced back.
- Add a new preparation patch:
[PATCH v2 1/3] nvme-hwmon: Return error on kzalloc failure
- Convert to allocating the nvme_smart_log structure instance instead of
  cache-aligning it. (@Christoph)

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
Cc: Alexey Malahov <Alexey.Malahov@baikalelectronics.ru>
Cc: Pavel Parkhomenko <Pavel.Parkhomenko@baikalelectronics.ru>
Cc: Thomas Bogendoerfer <tsbogend@alpha.franken.de>
Cc: linux-nvme@lists.infradead.org
Cc: linux-block@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

Serge Semin (3):
  nvme-hwmon: Return error on kzalloc failure
  nvme-hwmon: Kmalloc the NVME SMART log buffer
  block: sed-opal: Cache-line-align the cmd/resp buffers

 block/sed-opal.c          |  5 +++--
 drivers/nvme/host/hwmon.c | 34 ++++++++++++++++++++++++----------
 2 files changed, 27 insertions(+), 12 deletions(-)

-- 
2.37.3

[PATCH v2 1/3] nvme-hwmon: Return error on kzalloc failure

[Serge Semin]

Inability to allocate a buffer is a critical error which shouldn't be
tolerated since most likely the rest of the driver won't work correctly.
Thus instead of returning the zero status let's return the -ENOMEM error
if the nvme_hwmon_data structure instance couldn't be created.

Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

---

Changelog v2:
- New patch
---
 drivers/nvme/host/hwmon.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/nvme/host/hwmon.c b/drivers/nvme/host/hwmon.c
index 0a586d712920..1afb24a64145 100644
--- a/drivers/nvme/host/hwmon.c
+++ b/drivers/nvme/host/hwmon.c
@@ -230,7 +230,7 @@ int nvme_hwmon_init(struct nvme_ctrl *ctrl)
 
 	data = kzalloc(sizeof(*data), GFP_KERNEL);
 	if (!data)
-		return 0;
+		return -ENOMEM;
 
 	data->ctrl = ctrl;
 	mutex_init(&data->read_lock);
-- 
2.37.3

Re: [PATCH v2 1/3] nvme-hwmon: Return error on kzalloc failure

[Keith Busch]

On Fri, Sep 30, 2022 at 01:46:46AM +0300, Serge Semin wrote:
> Inability to allocate a buffer is a critical error which shouldn't be
> tolerated since most likely the rest of the driver won't work correctly.
> Thus instead of returning the zero status let's return the -ENOMEM error
> if the nvme_hwmon_data structure instance couldn't be created.

Nak for this one. The hwmon is not necessary for the rest of the driver to
function, so having the driver detach from the device seems a bit harsh. The
driver can participate in memory reclaim, so failing on a low memory condition
can make matters worse.

The rest looks good, though.

> @@ -230,7 +230,7 @@ int nvme_hwmon_init(struct nvme_ctrl *ctrl)
>  
>  	data = kzalloc(sizeof(*data), GFP_KERNEL);
>  	if (!data)
> -		return 0;
> +		return -ENOMEM;
>  
>  	data->ctrl = ctrl;
>  	mutex_init(&data->read_lock);
> -- 

Re: [PATCH v2 1/3] nvme-hwmon: Return error on kzalloc failure

[Serge Semin]

On Thu, Sep 29, 2022 at 05:53:43PM -0600, Keith Busch wrote:
> On Fri, Sep 30, 2022 at 01:46:46AM +0300, Serge Semin wrote:
> > Inability to allocate a buffer is a critical error which shouldn't be
> > tolerated since most likely the rest of the driver won't work correctly.
> > Thus instead of returning the zero status let's return the -ENOMEM error
> > if the nvme_hwmon_data structure instance couldn't be created.
> 

> Nak for this one. The hwmon is not necessary for the rest of the driver to
> function, so having the driver detach from the device seems a bit harsh.

Even if it is as you say, neither the method semantic nor the way it's
called imply that. Any failures except the allocation one are
perceived as erroneous.

> The
> driver can participate in memory reclaim, so failing on a low memory condition
> can make matters worse.

Yes it can, so can many other places in the driver utilizing kmalloc
with just GFP_KERNEL flag passed including on the same path as the
nvme_hwmon_init() execution. Kmalloc will make sure the reclaim is
either finished or executed in background anyway in all cases. Don't
really see why memory allocation failure is less worse in this case
than in many others in the same driver especially seeing as I said
above the method semantic doesn't imply the optional feature
detection. Moreover the allocated structure isn't huge at all. So
failing to allocate that would indeed mean problems with the memory
resource.

> 
> The rest looks good, though.

but you ok with kmalloc in the next line. Seems like contradicting.

@Christoph, what do you think about this?

-Sergey

> 
> > @@ -230,7 +230,7 @@ int nvme_hwmon_init(struct nvme_ctrl *ctrl)
> >  
> >  	data = kzalloc(sizeof(*data), GFP_KERNEL);
> >  	if (!data)
> > -		return 0;
> > +		return -ENOMEM;
> >  
> >  	data->ctrl = ctrl;
> >  	mutex_init(&data->read_lock);
> > -- 

Re: [PATCH v2 1/3] nvme-hwmon: Return error on kzalloc failure

[Keith Busch]

On Fri, Sep 30, 2022 at 12:52:47PM +0300, Serge Semin wrote:
> On Thu, Sep 29, 2022 at 05:53:43PM -0600, Keith Busch wrote:
> > On Fri, Sep 30, 2022 at 01:46:46AM +0300, Serge Semin wrote:
> > > Inability to allocate a buffer is a critical error which shouldn't be
> > > tolerated since most likely the rest of the driver won't work correctly.
> > > Thus instead of returning the zero status let's return the -ENOMEM error
> > > if the nvme_hwmon_data structure instance couldn't be created.
> > 
> 
> > Nak for this one. The hwmon is not necessary for the rest of the driver to
> > function, so having the driver detach from the device seems a bit harsh.
> 
> Even if it is as you say, neither the method semantic nor the way it's
> called imply that. Any failures except the allocation one are
> perceived as erroneous.

This is called by nvme_init_ctrl_finish(), and returns the error to
nvme_reset_work() only if it's < 0, which indicates we can't go on and the
driver unbinds.

This particular condition for hwmon is not something that prevents us from
making forward progress.
 
> > The
> > driver can participate in memory reclaim, so failing on a low memory condition
> > can make matters worse.
> 
> Yes it can, so can many other places in the driver utilizing kmalloc
> with just GFP_KERNEL flag passed including on the same path as the
> nvme_hwmon_init() execution. Kmalloc will make sure the reclaim is
> either finished or executed in background anyway in all cases. 

This path is in the first initialization before we've set up a namespace that
can be used as a reclaim destination.

> Don't
> really see why memory allocation failure is less worse in this case
> than in many others in the same driver especially seeing as I said

The other initialization kmalloc's are required to make forward progress toward
setting up a namespace. This one is not required.

Re: [PATCH v2 1/3] nvme-hwmon: Return error on kzalloc failure

[Serge Semin]

On Fri, Sep 30, 2022 at 08:57:02AM -0600, Keith Busch wrote:
> On Fri, Sep 30, 2022 at 12:52:47PM +0300, Serge Semin wrote:
> > On Thu, Sep 29, 2022 at 05:53:43PM -0600, Keith Busch wrote:
> > > On Fri, Sep 30, 2022 at 01:46:46AM +0300, Serge Semin wrote:
> > > > Inability to allocate a buffer is a critical error which shouldn't be
> > > > tolerated since most likely the rest of the driver won't work correctly.
> > > > Thus instead of returning the zero status let's return the -ENOMEM error
> > > > if the nvme_hwmon_data structure instance couldn't be created.
> > > 
> > 
> > > Nak for this one. The hwmon is not necessary for the rest of the driver to
> > > function, so having the driver detach from the device seems a bit harsh.
> > 
> > Even if it is as you say, neither the method semantic nor the way it's
> > called imply that. Any failures except the allocation one are
> > perceived as erroneous.
> 

> This is called by nvme_init_ctrl_finish(), and returns the error to
> nvme_reset_work() only if it's < 0, which indicates we can't go on and the
> driver unbinds.

That's obvious. One of the my question was that what makes the no
memory error different from the rest of the errors causing the
nvme_hwmon_init() method to fail? 

> 
> This particular condition for hwmon is not something that prevents us from
> making forward progress.

If you consider the hwmon functionality as optional (AFAIU you are),
then just ignore the return value no matter the reason. If the problem
caused the hwmon initialization process to fail turns to be critical
it will be raised in some other place which is required for the NVME
driver to work properly. Otherwise the hwmon module initialization may
still cause the probe procedure to halt, which makes it not optional.
That's what I meant when was saying about "the function and its
caller semantics not implying that".

>  
> > > The
> > > driver can participate in memory reclaim, so failing on a low memory condition
> > > can make matters worse.
> > 
> > Yes it can, so can many other places in the driver utilizing kmalloc
> > with just GFP_KERNEL flag passed including on the same path as the
> > nvme_hwmon_init() execution. Kmalloc will make sure the reclaim is
> > either finished or executed in background anyway in all cases. 
> 
> This path is in the first initialization before we've set up a namespace that
> can be used as a reclaim destination.
> 
> > Don't
> > really see why memory allocation failure is less worse in this case
> > than in many others in the same driver especially seeing as I said
> 
> The other initialization kmalloc's are required to make forward progress toward
> setting up a namespace. This one is not required.

Anyway what you say seems still contradicting. First you said that the
hwmon functionality was optional, but the only error being ignored was
the no-memory one which was very rare and turned to be not ignored in
the most of the other places. Second you got to accept the second
patch of the series, which introduced a one more kmalloc followed
right after the first one in the same function nvme_hwmon_init(). That
kmalloc failure wasn't ignored but caused the nvme_hwmon_init()
function to return an error. If you suggest to forget about the first
part (which IMO still counts, but AFAICS is a common pattern in the
NVME core driver, i.e. nvme_configure_apst() and
nvme_configure_host_options()), the second part still applies.

-Sergey

Re: [PATCH v2 1/3] nvme-hwmon: Return error on kzalloc failure

[Keith Busch]

On Tue, Oct 04, 2022 at 05:50:49PM +0300, Serge Semin wrote:
> > 
> > This particular condition for hwmon is not something that prevents us from
> > making forward progress.
> 
> If you consider the hwmon functionality as optional (AFAIU you are),
> then just ignore the return value no matter the reason. 

That is not an option. This function does IO, and the controller may not be
usable on the other side of that, which means initialization must abort. We
can't just ignore errors; we just don't need to report errors that don't
prevent forward progress.

> If the problem
> caused the hwmon initialization process to fail turns to be critical
> it will be raised in some other place which is required for the NVME
> driver to work properly. Otherwise the hwmon module initialization may
> still cause the probe procedure to halt, which makes it not optional.
> That's what I meant when was saying about "the function and its
> caller semantics not implying that".
> 
> >  
> > > > The
> > > > driver can participate in memory reclaim, so failing on a low memory condition
> > > > can make matters worse.
> > > 
> > > Yes it can, so can many other places in the driver utilizing kmalloc
> > > with just GFP_KERNEL flag passed including on the same path as the
> > > nvme_hwmon_init() execution. Kmalloc will make sure the reclaim is
> > > either finished or executed in background anyway in all cases. 
> > 
> > This path is in the first initialization before we've set up a namespace that
> > can be used as a reclaim destination.
> > 
> > > Don't
> > > really see why memory allocation failure is less worse in this case
> > > than in many others in the same driver especially seeing as I said
> > 
> > The other initialization kmalloc's are required to make forward progress toward
> > setting up a namespace. This one is not required.
> 
> Anyway what you say seems still contradicting. First you said that the
> hwmon functionality was optional, but the only error being ignored was
> the no-memory one which was very rare and turned to be not ignored in
> the most of the other places.

> Second you got to accept the second
> patch of the series, which introduced a one more kmalloc followed
> right after the first one in the same function nvme_hwmon_init(). 

My comments on this patch were intended to be applied to all similiarly added
uses.

Re: [PATCH v2 1/3] nvme-hwmon: Return error on kzalloc failure

[Serge Semin]

On Tue, Oct 04, 2022 at 11:33:44AM -0600, Keith Busch wrote:
> On Tue, Oct 04, 2022 at 05:50:49PM +0300, Serge Semin wrote:
> > > 
> > > This particular condition for hwmon is not something that prevents us from
> > > making forward progress.
> > 
> > If you consider the hwmon functionality as optional (AFAIU you are),
> > then just ignore the return value no matter the reason. 
> 
> That is not an option. This function does IO, and the controller may not be
> usable on the other side of that, which means initialization must abort. We
> can't just ignore errors; we just don't need to report errors that don't
> prevent forward progress.
> 
> > If the problem
> > caused the hwmon initialization process to fail turns to be critical
> > it will be raised in some other place which is required for the NVME
> > driver to work properly. Otherwise the hwmon module initialization may
> > still cause the probe procedure to halt, which makes it not optional.
> > That's what I meant when was saying about "the function and its
> > caller semantics not implying that".
> > 
> > >  
> > > > > The
> > > > > driver can participate in memory reclaim, so failing on a low memory condition
> > > > > can make matters worse.
> > > > 
> > > > Yes it can, so can many other places in the driver utilizing kmalloc
> > > > with just GFP_KERNEL flag passed including on the same path as the
> > > > nvme_hwmon_init() execution. Kmalloc will make sure the reclaim is
> > > > either finished or executed in background anyway in all cases. 
> > > 
> > > This path is in the first initialization before we've set up a namespace that
> > > can be used as a reclaim destination.
> > > 
> > > > Don't
> > > > really see why memory allocation failure is less worse in this case
> > > > than in many others in the same driver especially seeing as I said
> > > 
> > > The other initialization kmalloc's are required to make forward progress toward
> > > setting up a namespace. This one is not required.
> > 
> > Anyway what you say seems still contradicting. First you said that the
> > hwmon functionality was optional, but the only error being ignored was
> > the no-memory one which was very rare and turned to be not ignored in
> > the most of the other places.
> 
> > Second you got to accept the second
> > patch of the series, which introduced a one more kmalloc followed
> > right after the first one in the same function nvme_hwmon_init(). 
> 

> My comments on this patch were intended to be applied to all similiarly added
> uses.

How could I've possibly got that from your "The rest looks good,
though." and nacking only this patch with the message "The hwmon is
not necessary for the rest of the driver..." ?

Anyway due to all these uncertainties it's better to have a second
opinion on the patches before re-spining the series.

@Christoph, since you've already started reviewing the pathchset could
you have a look at the patches #1 and #2 of the series? Please note
the @Keith' comments regarding the memory allocation failure handling
in there.

-Sergey

[PATCH v2 2/3] nvme-hwmon: Kmalloc the NVME SMART log buffer

[Serge Semin]

Recent commit 52fde2c07da6 ("nvme: set dma alignment to dword") has caused
a regression on our platform. It turned out that the nvme_get_log() method
invocation caused the nvme_hwmon_data structure instance corruption. In
particular the nvme_hwmon_data.ctrl pointer was overwritten either with
zeros or with garbage. After some researches we discovered that the
problem happened even before the actual NVME DMA execution, but during the
buffer mapping. Since our platform was DMA-noncoherent the mapping implied
the cache-lines invalidations or write-backs depending on the
DMA-direction parameter. In case of the NVME SMART log getting the DMA was
performed from-device-to-memory, thus the cache-invalidation was activated
during the buffer mapping. Since the log-buffer wasn't cache-line aligned
the cache-invalidation caused the neighbour data discard. The neighbouring
data turned to be the data surrounding the buffer in the framework of the
nvme_hwmon_data structure.

In order to fix that we need to make sure that the whole log-buffer is
defined within the cache-line-aligned memory region so the
cache-invalidation procedure wouldn't involve the adjacent data. One of
the option to guarantee that is to kmalloc the DMA-buffer [1]. Seeing the
rest of the NVME core driver prefer that method it has been chosen to fix
this problem too.

Note after a deeper researches we found out that the denoted commit wasn't
a root cause of the problem. It just revealed the invalidity by activating
the DMA-based NVME SMART log getting performed in the framework of the
NVME hwmon driver. The problem was here since the initial commit of the
driver.

[1] Documentation/core-api/dma-api-howto.rst

Fixes: 400b6a7b13a3 ("nvme: Add hardware monitoring support")
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

---

Folks, I've thoroughly studied the whole NVME subsystem looking for
similar problems. Turned out there is one more place which may cause the
same issue. It's connected with the opal_dev.{cmd,req} buffers passed to
the nvme_sec_submit() method. The rest of the buffers involved in the NVME
DMA are either allocated by kmalloc (must be cache-line-aligned by design)
or bounced-buffered if allocated on the stack (see the blk_rq_map_kern()
method implementation). I am still not fully sure regarding the buffers
coming from user-space though, but AFAICS based on our
DMA-buffers-alignment sanity check procedure they haven't been detected as
cache-unaligned so far.

Changelog v2:
- Convert to allocating the nvme_smart_log structure instance instead of
  cache-aligning it. (@Christoph)
---
 drivers/nvme/host/hwmon.c | 32 +++++++++++++++++++++++---------
 1 file changed, 23 insertions(+), 9 deletions(-)

diff --git a/drivers/nvme/host/hwmon.c b/drivers/nvme/host/hwmon.c
index 1afb24a64145..654309767e76 100644
--- a/drivers/nvme/host/hwmon.c
+++ b/drivers/nvme/host/hwmon.c
@@ -12,7 +12,7 @@
 
 struct nvme_hwmon_data {
 	struct nvme_ctrl *ctrl;
-	struct nvme_smart_log log;
+	struct nvme_smart_log *log;
 	struct mutex read_lock;
 };
 
@@ -60,14 +60,14 @@ static int nvme_set_temp_thresh(struct nvme_ctrl *ctrl, int sensor, bool under,
 static int nvme_hwmon_get_smart_log(struct nvme_hwmon_data *data)
 {
 	return nvme_get_log(data->ctrl, NVME_NSID_ALL, NVME_LOG_SMART, 0,
-			   NVME_CSI_NVM, &data->log, sizeof(data->log), 0);
+			   NVME_CSI_NVM, data->log, sizeof(*data->log), 0);
 }
 
 static int nvme_hwmon_read(struct device *dev, enum hwmon_sensor_types type,
 			   u32 attr, int channel, long *val)
 {
 	struct nvme_hwmon_data *data = dev_get_drvdata(dev);
-	struct nvme_smart_log *log = &data->log;
+	struct nvme_smart_log *log = data->log;
 	int temp;
 	int err;
 
@@ -163,7 +163,7 @@ static umode_t nvme_hwmon_is_visible(const void *_data,
 	case hwmon_temp_max:
 	case hwmon_temp_min:
 		if ((!channel && data->ctrl->wctemp) ||
-		    (channel && data->log.temp_sensor[channel - 1])) {
+		    (channel && data->log->temp_sensor[channel - 1])) {
 			if (data->ctrl->quirks &
 			    NVME_QUIRK_NO_TEMP_THRESH_CHANGE)
 				return 0444;
@@ -176,7 +176,7 @@ static umode_t nvme_hwmon_is_visible(const void *_data,
 		break;
 	case hwmon_temp_input:
 	case hwmon_temp_label:
-		if (!channel || data->log.temp_sensor[channel - 1])
+		if (!channel || data->log->temp_sensor[channel - 1])
 			return 0444;
 		break;
 	default:
@@ -232,14 +232,19 @@ int nvme_hwmon_init(struct nvme_ctrl *ctrl)
 	if (!data)
 		return -ENOMEM;
 
+	data->log = kzalloc(sizeof(*data->log), GFP_KERNEL);
+	if (!data->log) {
+		err = -ENOMEM;
+		goto err_free_data;
+	}
+
 	data->ctrl = ctrl;
 	mutex_init(&data->read_lock);
 
 	err = nvme_hwmon_get_smart_log(data);
 	if (err) {
 		dev_warn(dev, "Failed to read smart log (error %d)\n", err);
-		kfree(data);
-		return err;
+		goto err_free_log;
 	}
 
 	hwmon = hwmon_device_register_with_info(dev, "nvme",
@@ -247,11 +252,19 @@ int nvme_hwmon_init(struct nvme_ctrl *ctrl)
 						NULL);
 	if (IS_ERR(hwmon)) {
 		dev_warn(dev, "Failed to instantiate hwmon device\n");
-		kfree(data);
-		return PTR_ERR(hwmon);
+		err = PTR_ERR(hwmon);
+		goto err_free_log;
 	}
 	ctrl->hwmon_device = hwmon;
 	return 0;
+
+err_free_log:
+	kfree(data->log);
+
+err_free_data:
+	kfree(data);
+
+	return err;
 }
 
 void nvme_hwmon_exit(struct nvme_ctrl *ctrl)
@@ -262,6 +275,7 @@ void nvme_hwmon_exit(struct nvme_ctrl *ctrl)
 
 		hwmon_device_unregister(ctrl->hwmon_device);
 		ctrl->hwmon_device = NULL;
+		kfree(data->log);
 		kfree(data);
 	}
 }
-- 
2.37.3

Re: [PATCH v2 2/3] nvme-hwmon: Kmalloc the NVME SMART log buffer

[Christoph Hellwig]

Thanks,

applied to nvme-6.1.

Re: [PATCH v2 2/3] nvme-hwmon: Kmalloc the NVME SMART log buffer

[Serge Semin]

Hello Christoph

On Mon, Oct 17, 2022 at 09:18:32AM +0200, Christoph Hellwig wrote:
> Thanks,
> 
> applied to nvme-6.1.

Please note the applied patch doesn't comply with the Keith' notes
Link: https://lore.kernel.org/linux-nvme/YzxueNRODpry8L0%2F@kbusch-mbp.dhcp.thefacebook.com/
Meanwhile without patch #1 (having only the accepted by you patch
applied) the NVME hwmon init now seems contradicting: it ignores one
kmalloc failure (returns zero) but fails on another one (returns
-ENOMEM). I asked you to have a look at the patches #1 and #2 of the
series
Link: https://lore.kernel.org/linux-nvme/20221007100134.faaekmuqyd5vy34m@mobilestation/
and give your opinion whether the re-spin was required: take the
Keith' notes or keep the patches as is. Could you please clarify the
situation?

-Sergey

Re: [PATCH v2 2/3] nvme-hwmon: Kmalloc the NVME SMART log buffer

[Christoph Hellwig]

On Mon, Oct 17, 2022 at 07:16:56PM +0300, Serge Semin wrote:
> Please note the applied patch doesn't comply with the Keith' notes
> Link: https://lore.kernel.org/linux-nvme/YzxueNRODpry8L0%2F@kbusch-mbp.dhcp.thefacebook.com/
> Meanwhile without patch #1 (having only the accepted by you patch
> applied) the NVME hwmon init now seems contradicting: it ignores one
> kmalloc failure (returns zero) but fails on another one (returns
> -ENOMEM). I asked you to have a look at the patches #1 and #2 of the
> series
> Link: https://lore.kernel.org/linux-nvme/20221007100134.faaekmuqyd5vy34m@mobilestation/
> and give your opinion whether the re-spin was required: take the
> Keith' notes or keep the patches as is. Could you please clarify the
> situation?

I'll fix this patch up to follow the recommendation from Keith, I somehow
thought this was already done.

[PATCH v2 3/3] block: sed-opal: Cache-line-align the cmd/resp buffers

[Serge Semin]

In accordance with [1] the DMA-able memory buffers must be
cacheline-aligned otherwise the cache writing-back and invalidation
performed during the mapping may cause the adjacent data being lost. It's
specifically required for the DMA-noncoherent platforms. Seeing the
opal_dev.{cmd,resp} buffers are used for DMAs in the NVME and SCSI/SD
drivers in framework of the nvme_sec_submit() and sd_sec_submit() methods
respectively we must make sure the passed buffers are cacheline-aligned to
prevent the denoted problem.

[1] Documentation/core-api/dma-api.rst

Fixes: 455a7b238cd6 ("block: Add Sed-opal library")
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
---
 block/sed-opal.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/block/sed-opal.c b/block/sed-opal.c
index 9700197000f2..222acbd1f03a 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -73,6 +73,7 @@ struct parsed_resp {
 	struct opal_resp_tok toks[MAX_TOKS];
 };
 
+/* Presumably DMA-able buffers must be cache-aligned */
 struct opal_dev {
 	bool supported;
 	bool mbr_enabled;
@@ -88,8 +89,8 @@ struct opal_dev {
 	u64 lowest_lba;
 
 	size_t pos;
-	u8 cmd[IO_BUFFER_LENGTH];
-	u8 resp[IO_BUFFER_LENGTH];
+	u8 cmd[IO_BUFFER_LENGTH] ____cacheline_aligned;
+	u8 resp[IO_BUFFER_LENGTH] ____cacheline_aligned;
 
 	struct parsed_resp parsed;
 	size_t prev_d_len;
-- 
2.37.3

Re: [PATCH v2 3/3] block: sed-opal: Cache-line-align the cmd/resp buffers

[Jonathan Derrick]

Hi

On 9/29/2022 4:46 PM, Serge Semin wrote:
> In accordance with [1] the DMA-able memory buffers must be
> cacheline-aligned otherwise the cache writing-back and invalidation
> performed during the mapping may cause the adjacent data being lost. It's
> specifically required for the DMA-noncoherent platforms. Seeing the
> opal_dev.{cmd,resp} buffers are used for DMAs in the NVME and SCSI/SD
> drivers in framework of the nvme_sec_submit() and sd_sec_submit() methods
> respectively we must make sure the passed buffers are cacheline-aligned to
> prevent the denoted problem.
> 
> [1] Documentation/core-api/dma-api.rst
> 
> Fixes: 455a7b238cd6 ("block: Add Sed-opal library")
> Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> ---
>   block/sed-opal.c | 5 +++--
>   1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/block/sed-opal.c b/block/sed-opal.c
> index 9700197000f2..222acbd1f03a 100644
> --- a/block/sed-opal.c
> +++ b/block/sed-opal.c
> @@ -73,6 +73,7 @@ struct parsed_resp {
>   	struct opal_resp_tok toks[MAX_TOKS];
>   };
>   
> +/* Presumably DMA-able buffers must be cache-aligned */
>   struct opal_dev {
>   	bool supported;
>   	bool mbr_enabled;
> @@ -88,8 +89,8 @@ struct opal_dev {
>   	u64 lowest_lba;
>   
>   	size_t pos;
> -	u8 cmd[IO_BUFFER_LENGTH];
> -	u8 resp[IO_BUFFER_LENGTH];
> +	u8 cmd[IO_BUFFER_LENGTH] ____cacheline_aligned;
> +	u8 resp[IO_BUFFER_LENGTH] ____cacheline_aligned;
I'm with Christoph on this one.
When I see ____cacheline_aligned, I assume its for performance reasons, 
not to work around a DMA limitation. Can we instead kmalloc (which 
provides alignment) these buffers to make it more clear? May want to add 
that same comment pointing out some architectures require these dma 
targets to be cache aligned.


>   
>   	struct parsed_resp parsed;
>   	size_t prev_d_len;

Re: [PATCH v2 3/3] block: sed-opal: Cache-line-align the cmd/resp buffers

[Serge Semin]

On Mon, Oct 03, 2022 at 12:24:08PM -0600, Jonathan Derrick wrote:
> Hi
> 
> On 9/29/2022 4:46 PM, Serge Semin wrote:
> > In accordance with [1] the DMA-able memory buffers must be
> > cacheline-aligned otherwise the cache writing-back and invalidation
> > performed during the mapping may cause the adjacent data being lost. It's
> > specifically required for the DMA-noncoherent platforms. Seeing the
> > opal_dev.{cmd,resp} buffers are used for DMAs in the NVME and SCSI/SD
> > drivers in framework of the nvme_sec_submit() and sd_sec_submit() methods
> > respectively we must make sure the passed buffers are cacheline-aligned to
> > prevent the denoted problem.
> > 
> > [1] Documentation/core-api/dma-api.rst
> > 
> > Fixes: 455a7b238cd6 ("block: Add Sed-opal library")
> > Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>
> > ---
> >   block/sed-opal.c | 5 +++--
> >   1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/block/sed-opal.c b/block/sed-opal.c
> > index 9700197000f2..222acbd1f03a 100644
> > --- a/block/sed-opal.c
> > +++ b/block/sed-opal.c
> > @@ -73,6 +73,7 @@ struct parsed_resp {
> >   	struct opal_resp_tok toks[MAX_TOKS];
> >   };
> > +/* Presumably DMA-able buffers must be cache-aligned */
> >   struct opal_dev {
> >   	bool supported;
> >   	bool mbr_enabled;
> > @@ -88,8 +89,8 @@ struct opal_dev {
> >   	u64 lowest_lba;
> >   	size_t pos;
> > -	u8 cmd[IO_BUFFER_LENGTH];
> > -	u8 resp[IO_BUFFER_LENGTH];
> > +	u8 cmd[IO_BUFFER_LENGTH] ____cacheline_aligned;
> > +	u8 resp[IO_BUFFER_LENGTH] ____cacheline_aligned;

> I'm with Christoph on this one.
> When I see ____cacheline_aligned, I assume its for performance reasons, not
> to work around a DMA limitation. Can we instead kmalloc (which provides
> alignment) these buffers to make it more clear? May want to add that same
> comment pointing out some architectures require these dma targets to be
> cache aligned.

Ok. I'll resend v3 with these buffers being kmalloc'ed.

Please note the SED OPAL entry of the MAINTAINTER list contains your
intel-email address, which bounces back the messages (so does the
Revanth' one). I'll add your new address to my patchset' "To"-list,
but if you want to get new OPAL-related patches sent directly to your
linux.dev email address the entry should be updated.

-Sergey

> 
> 
> >   	struct parsed_resp parsed;
> >   	size_t prev_d_len;

[PATCH v3] block: sed-opal: kmalloc the cmd/resp buffers

[Serge Semin]

In accordance with [1] the DMA-able memory buffers must be
cacheline-aligned otherwise the cache writing-back and invalidation
performed during the mapping may cause the adjacent data being lost. It's
specifically required for the DMA-noncoherent platforms [2]. Seeing the
opal_dev.{cmd,resp} buffers are implicitly used for DMAs in the NVME and
SCSI/SD drivers in framework of the nvme_sec_submit() and sd_sec_submit()
methods respectively they must be cacheline-aligned to prevent the denoted
problem. One of the option to guarantee that is to kmalloc the buffers
[2]. Let's explicitly allocate them then instead of embedding into the
opal_dev structure instance.

Note this fix was inspired by the commit c94b7f9bab22 ("nvme-hwmon:
kmalloc the NVME SMART log buffer").

[1] Documentation/core-api/dma-api.rst
[2] Documentation/core-api/dma-api-howto.rst

Fixes: 455a7b238cd6 ("block: Add Sed-opal library")
Signed-off-by: Serge Semin <Sergey.Semin@baikalelectronics.ru>

---

Folks the NVME-part of the patchset has already been merged in
Link: https://lore.kernel.org/linux-nvme/20220929224648.8997-1-Sergey.Semin@baikalelectronics.ru/
This modification is only leftover of the original series. So I've resent
it as a separate patch.

Link: https://lore.kernel.org/linux-nvme/20220929224648.8997-4-Sergey.Semin@baikalelectronics.ru/
Changelog v3:
- Convert to allocating the cmd-/resp-buffers instead of cache-aligning
  them. (@Jonathan)
- Resubmit the patch separately from the original series.
- Rebase onto the kernel 6.1-rc3
---
 block/sed-opal.c | 32 ++++++++++++++++++++++++++++----
 1 file changed, 28 insertions(+), 4 deletions(-)

diff --git a/block/sed-opal.c b/block/sed-opal.c
index 2c5327a0543a..9bdb833e5817 100644
--- a/block/sed-opal.c
+++ b/block/sed-opal.c
@@ -87,8 +87,8 @@ struct opal_dev {
 	u64 lowest_lba;
 
 	size_t pos;
-	u8 cmd[IO_BUFFER_LENGTH];
-	u8 resp[IO_BUFFER_LENGTH];
+	u8 *cmd;
+	u8 *resp;
 
 	struct parsed_resp parsed;
 	size_t prev_d_len;
@@ -2175,6 +2175,8 @@ void free_opal_dev(struct opal_dev *dev)
 		return;
 
 	clean_opal_dev(dev);
+	kfree(dev->resp);
+	kfree(dev->cmd);
 	kfree(dev);
 }
 EXPORT_SYMBOL(free_opal_dev);
@@ -2187,6 +2189,18 @@ struct opal_dev *init_opal_dev(void *data, sec_send_recv *send_recv)
 	if (!dev)
 		return NULL;
 
+	/*
+	 * Presumably DMA-able buffers must be cache-aligned. Kmalloc makes
+	 * sure the allocated buffer is DMA-safe in that regard.
+	 */
+	dev->cmd = kmalloc(IO_BUFFER_LENGTH, GFP_KERNEL);
+	if (!dev->cmd)
+		goto err_free_dev;
+
+	dev->resp = kmalloc(IO_BUFFER_LENGTH, GFP_KERNEL);
+	if (!dev->resp)
+		goto err_free_cmd;
+
 	INIT_LIST_HEAD(&dev->unlk_lst);
 	mutex_init(&dev->dev_lock);
 	dev->flags = 0;
@@ -2194,11 +2208,21 @@ struct opal_dev *init_opal_dev(void *data, sec_send_recv *send_recv)
 	dev->send_recv = send_recv;
 	if (check_opal_support(dev) != 0) {
 		pr_debug("Opal is not supported on this device\n");
-		kfree(dev);
-		return NULL;
+		goto err_free_resp;
 	}
 
 	return dev;
+
+err_free_resp:
+	kfree(dev->resp);
+
+err_free_cmd:
+	kfree(dev->cmd);
+
+err_free_dev:
+	kfree(dev);
+
+	return NULL;
 }
 EXPORT_SYMBOL(init_opal_dev);
 
-- 
2.38.0

Re: [PATCH v3] block: sed-opal: kmalloc the cmd/resp buffers

[Christoph Hellwig]

Looks good:

Reviewed-by: Christoph Hellwig <hch@lst.de>

Re: [PATCH v3] block: sed-opal: kmalloc the cmd/resp buffers

[Jens Axboe]

On Mon, 7 Nov 2022 23:39:44 +0300, Serge Semin wrote:
> In accordance with [1] the DMA-able memory buffers must be
> cacheline-aligned otherwise the cache writing-back and invalidation
> performed during the mapping may cause the adjacent data being lost. It's
> specifically required for the DMA-noncoherent platforms [2]. Seeing the
> opal_dev.{cmd,resp} buffers are implicitly used for DMAs in the NVME and
> SCSI/SD drivers in framework of the nvme_sec_submit() and sd_sec_submit()
> methods respectively they must be cacheline-aligned to prevent the denoted
> problem. One of the option to guarantee that is to kmalloc the buffers
> [2]. Let's explicitly allocate them then instead of embedding into the
> opal_dev structure instance.
> 
> [...]

Applied, thanks!

[1/1] block: sed-opal: kmalloc the cmd/resp buffers
      (no commit info)

Best regards,
-- 
Jens Axboe