[PATCH] nvme-pci: prevent SK Hynix PE8000 from using Write Zeroes command

[PATCH] nvme-pci: prevent SK Hynix PE8000 from using Write Zeroes command

[Gopal Tiwari]

For SK Hynix PE8000 WriteZerocommands exceed typical I/O MDTS
(Maximum Data Transfer Size) supported by the hardware. Which
intern fail and generate following erros :

nvme nvme0: I/O 0 QID 96 timeout, aborting
nvme nvme0: Abort status: 0x371
nvme nvme0: Shutdown timeout set to 15 seconds
nvme nvme0: 128/0/0 default/read/poll queues
nvme nvme0: I/O 0 QID 96 timeout, disable controller
nvme nvme0: I/O 0 QID 0 timeout, disable controller
nvme nvme0: Device shutdown incomplete; abort shutdown
nvme nvme0: failed to mark controller live state
nvme nvme0: Removing after probe failure status: -19

Firmware unable to complete any outstanding commands
due to lack of resources, therefore placing NVMe drive
in a condition that is unable to further process WriteZerocommands.

fixing by disabling Write zeroes.

Signed-off-by: Gopal Tiwari <gtiwari@redhat.com>
---
 drivers/nvme/host/pci.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 6bad4d4dcdf0..aca67728700e 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -3257,6 +3257,8 @@ static const struct pci_device_id nvme_id_table[] = {
 				NVME_QUIRK_IGNORE_DEV_SUBNQN, },
 	{ PCI_DEVICE(0x1c5c, 0x1504),   /* SK Hynix PC400 */
 		.driver_data = NVME_QUIRK_DISABLE_WRITE_ZEROES, },
+	{ PCI_DEVICE(0x1c5c, 0x2839),  /* SK Hynix PE8000 U.3 NVMe storage */
+		.driver_data = NVME_QUIRK_DISABLE_WRITE_ZEROES, },
 	{ PCI_DEVICE(0x15b7, 0x2001),   /*  Sandisk Skyhawk */
 		.driver_data = NVME_QUIRK_DISABLE_WRITE_ZEROES, },
 	{ PCI_DEVICE(0x1d97, 0x2263),   /* SPCC */
-- 
2.26.2


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-pci: prevent SK Hynix PE8000 from using Write Zeroes command

[Chaitanya Kulkarni]

Gopal,

On 2/8/21 21:06, Gopal Tiwari wrote:
> For SK Hynix PE8000 WriteZerocommands exceed typical I/O MDTS
> (Maximum Data Transfer Size) supported by the hardware. Which
> intern fail and generate following erros :
>
> nvme nvme0: I/O 0 QID 96 timeout, aborting
> nvme nvme0: Abort status: 0x371
> nvme nvme0: Shutdown timeout set to 15 seconds
> nvme nvme0: 128/0/0 default/read/poll queues
> nvme nvme0: I/O 0 QID 96 timeout, disable controller
> nvme nvme0: I/O 0 QID 0 timeout, disable controller
> nvme nvme0: Device shutdown incomplete; abort shutdown
> nvme nvme0: failed to mark controller live state
> nvme nvme0: Removing after probe failure status: -19
>
> Firmware unable to complete any outstanding commands
> due to lack of resources, therefore placing NVMe drive
> in a condition that is unable to further process WriteZerocommands.
>
> fixing by disabling Write zeroes.

Please consider following commit log, can be used at the time applying the
patch:-

For SK Hynix PE8000 NVMe Write Zeroes Command exceed typical I/O MDTS
(Maximum Data Transfer Size) supported by the hardware. That intern
fail and generate following errors :

nvme nvme0: I/O 0 QID 96 timeout, aborting
nvme nvme0: Abort status: 0x371
nvme nvme0: Shutdown timeout set to 15 seconds
nvme nvme0: 128/0/0 default/read/poll queues
nvme nvme0: I/O 0 QID 96 timeout, disable controller
nvme nvme0: I/O 0 QID 0 timeout, disable controller
nvme nvme0: Device shutdown incomplete; abort shutdown
nvme nvme0: failed to mark controller live state
nvme nvme0: Removing after probe failure status: -19

Firmware is unable to complete any outstanding commands due to lack of
resources, therefore placing NVMe drive in a condition that is unable to
further process write-zeores
command.                                                                 

Add NVME_QUIRK_DISABLE_WRITE_ZEROES quirk for SK Hynix PE8000 that fixes
the problem.


With the modified commit log, looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-pci: prevent SK Hynix PE8000 from using Write Zeroes command

[Gopal Tiwari]

Thanks Chaitanya, Sure. 

Regards,
Gopal 


----- Original Message -----
From: "Chaitanya Kulkarni" <Chaitanya.Kulkarni@wdc.com>
To: "Gopal Tiwari" <gtiwari@redhat.com>, hch@lst.de, kbusch@kernel.org, linux-nvme@lists.infradead.org
Sent: Tuesday, February 9, 2021 11:08:03 AM
Subject: Re: [PATCH] nvme-pci: prevent SK Hynix PE8000 from using Write Zeroes command

Gopal,

On 2/8/21 21:06, Gopal Tiwari wrote:
> For SK Hynix PE8000 WriteZerocommands exceed typical I/O MDTS
> (Maximum Data Transfer Size) supported by the hardware. Which
> intern fail and generate following erros :
>
> nvme nvme0: I/O 0 QID 96 timeout, aborting
> nvme nvme0: Abort status: 0x371
> nvme nvme0: Shutdown timeout set to 15 seconds
> nvme nvme0: 128/0/0 default/read/poll queues
> nvme nvme0: I/O 0 QID 96 timeout, disable controller
> nvme nvme0: I/O 0 QID 0 timeout, disable controller
> nvme nvme0: Device shutdown incomplete; abort shutdown
> nvme nvme0: failed to mark controller live state
> nvme nvme0: Removing after probe failure status: -19
>
> Firmware unable to complete any outstanding commands
> due to lack of resources, therefore placing NVMe drive
> in a condition that is unable to further process WriteZerocommands.
>
> fixing by disabling Write zeroes.

Please consider following commit log, can be used at the time applying the
patch:-

For SK Hynix PE8000 NVMe Write Zeroes Command exceed typical I/O MDTS
(Maximum Data Transfer Size) supported by the hardware. That intern
fail and generate following errors :

nvme nvme0: I/O 0 QID 96 timeout, aborting
nvme nvme0: Abort status: 0x371
nvme nvme0: Shutdown timeout set to 15 seconds
nvme nvme0: 128/0/0 default/read/poll queues
nvme nvme0: I/O 0 QID 96 timeout, disable controller
nvme nvme0: I/O 0 QID 0 timeout, disable controller
nvme nvme0: Device shutdown incomplete; abort shutdown
nvme nvme0: failed to mark controller live state
nvme nvme0: Removing after probe failure status: -19

Firmware is unable to complete any outstanding commands due to lack of
resources, therefore placing NVMe drive in a condition that is unable to
further process write-zeores
command.                                                                 

Add NVME_QUIRK_DISABLE_WRITE_ZEROES quirk for SK Hynix PE8000 that fixes
the problem.


With the modified commit log, looks good.

Reviewed-by: Chaitanya Kulkarni <chaitanya.kulkarni@wdc.com>


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-pci: prevent SK Hynix PE8000 from using Write Zeroes command

[Christoph Hellwig]

On Tue, Feb 09, 2021 at 10:29:02AM +0530, Gopal Tiwari wrote:
> For SK Hynix PE8000 WriteZerocommands exceed typical I/O MDTS
> (Maximum Data Transfer Size) supported by the hardware. Which
> intern fail and generate following erros :

Well, if it works under MDTS please add a new quirk that follows
MDTS for Write Zeroes.  In fact I wonder if we should just do
that by default to be on the safe side.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-pci: prevent SK Hynix PE8000 from using Write Zeroes command

[Gopal Tiwari]

Hi Christoph, 

Thanks for the response,

----- Original Message -----
From: "Christoph Hellwig" <hch@infradead.org>
To: "Gopal Tiwari" <gtiwari@redhat.com>
Cc: kbusch@kernel.org, hch@lst.de, linux-nvme@lists.infradead.org
Sent: Wednesday, February 10, 2021 3:42:29 PM
Subject: Re: [PATCH] nvme-pci: prevent SK Hynix PE8000 from using Write Zeroes command

On Tue, Feb 09, 2021 at 10:29:02AM +0530, Gopal Tiwari wrote:
>> For SK Hynix PE8000 WriteZerocommands exceed typical I/O MDTS
>> (Maximum Data Transfer Size) supported by the hardware. Which
>> intern fail and generate following erros :

>Well, if it works under MDTS please add a new quirk that follows
MDTS for Write Zeroes.

This was the feedback from the hardware guys, We've not tried, as the safe option would be to disable 
write zero's as spec says 

I could see from the identify controller details 

  [0:0] : 0	Single Port

mdts      : 7 <<---------- 
cntlid    : 0
ver       : 0x10300

"This field does not apply to commands that do not transfer
data between host-accessible memory and the controller (e.g., the Verify command,
the Write Uncorrectable command, and the Write Zeroes command); there is no
maximum data transfer size for those commands. "

>  In fact I wonder if we should just do that by default to be on the safe side.
Can you please help me in understanding this ..  

Thanks & regards,
Gopal Tiwari 

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-pci: prevent SK Hynix PE8000 from using Write Zeroes command

[Christoph Hellwig]

MDTS vs Write Zeroes is a common misconception.  We had that discussion
in the NVMe working group, and while the text should pretty clear that
MDTS only applies to data transfers many implementators did not understand
that, which is why we added the clarification you quoted.

Also the next NVMe spec will allow devices to advertise an explicit
Write Zeroes limit, take a look at TP4040 from the "NVM Express 1.4 Ratified
TPs" at https://nvmexpress.org/developers/nvme-specification/.

So I think limiting to MDTS unless the new WZSL is set might be a fail
safe option, even if it is more pessimistic than what the spec says.

I'd also love to retest most Write Zeroes quirks with that in place.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-pci: prevent SK Hynix PE8000 from using Write Zeroes command

[Chaitanya Kulkarni]

On 2/10/21 5:20 AM, Christoph Hellwig wrote:
> MDTS vs Write Zeroes is a common misconception.  We had that discussion
> in the NVMe working group, and while the text should pretty clear that
> MDTS only applies to data transfers many implementators did not understand
> that, which is why we added the clarification you quoted.
>
> Also the next NVMe spec will allow devices to advertise an explicit
> Write Zeroes limit, take a look at TP4040 from the "NVM Express 1.4 Ratified
> TPs" at https://nvmexpress.org/developers/nvme-specification/.
>
> So I think limiting to MDTS unless the new WZSL is set might be a fail
> safe option, even if it is more pessimistic than what the spec says.
>
> I'd also love to retest most Write Zeroes quirks with that in place.

Do you prefer some variant of the following patch (totally untested)? OR
something else ?

Also, I think we should gradually roll out the MDTS replacement and
still keep the
write-zeroes quirk unless you are okay to just replace at one go.

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index d77f3f26d8d3..d5ca27608881 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1167,7 +1167,8 @@ static int nvme_submit_user_cmd(struct
request_queue *q,
                if (ret)
                        goto out;
                bio = req->bio;
-               bio_set_dev(bio, bdev);
+               if(bdev)
+                       bio_set_dev(bio, bdev);
                if (bdev && meta_buffer && meta_len) {
                        meta = nvme_add_user_metadata(bio, meta_buffer,
meta_len,
                                        meta_seed, write);
@@ -1967,6 +1968,7 @@ static void nvme_config_write_zeroes(struct
gendisk *disk, struct nvme_ns *ns)
        u64 max_blocks;
 
        if (!(ns->ctrl->oncs & NVME_CTRL_ONCS_WRITE_ZEROES) ||
+           !(ns->ctrl->quirks & NVME_QUIRK_USE_MDTS_WRITE_ZEROES) ||
            (ns->ctrl->quirks & NVME_QUIRK_DISABLE_WRITE_ZEROES))
                return;
        /*
@@ -1979,10 +1981,15 @@ static void nvme_config_write_zeroes(struct
gendisk *disk, struct nvme_ns *ns)
         * configured based on the controller's MDTS field in the
         * nvme_init_identify() if available.
         */
-       if (ns->ctrl->max_hw_sectors == UINT_MAX)
-               max_blocks = (u64)USHRT_MAX + 1;
-       else
-               max_blocks = ns->ctrl->max_hw_sectors + 1;
+
+       if (!(ns->ctrl->quirks & NVME_QUIRK_USE_MDTS_WRITE_ZEROES)) {
+               if (ns->ctrl->max_hw_sectors == UINT_MAX)
+                       max_blocks = (u64)USHRT_MAX + 1;
+               else
+                       max_blocks = ns->ctrl->max_hw_sectors + 1;
+       } else {
+               max_blocks = ns->ctrl->max_mdts_hw_sectors;
+       }
 
        blk_queue_max_write_zeroes_sectors(disk->queue,
                                           nvme_lba_to_sect(ns,
max_blocks));
@@ -3141,6 +3148,11 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
        ctrl->max_hw_sectors =
                min_not_zero(ctrl->max_hw_sectors, max_hw_sectors);
 
+       /* explicitely set the mdts value to 0 */
+       ctrl->max_mdts_hw_sectors = 0;
+       if (id->mdts)
+               ctrl->max_mdts_hw_sectors =  1 << (id->mdts + page_shift
- 9);
+
        nvme_set_queue_limits(ctrl, ctrl->admin_q);
        ctrl->sgls = le32_to_cpu(id->sgls);
        ctrl->kas = le16_to_cpu(id->kas);
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 0a73e78f09c8..7b60b2196d23 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -144,6 +144,11 @@ enum nvme_quirks {
         * NVMe 1.3 compliance.
         */
        NVME_QUIRK_NO_NS_DESC_LIST              = (1 << 15),
+       /*
+        * The controller doesn't allow the write zeores command of size
+        * more than reported by the MDTS
+        */
+       NVME_QUIRK_USE_MDTS_WRITE_ZEROES        = (1 << 16)
 };
 
 /*
@@ -268,6 +273,7 @@ struct nvme_ctrl {
 
        u64 cap;
        u32 max_hw_sectors;
+       u32 max_mdts_hw_sectors;
        u32 max_segments;
        u32 max_integrity_segments;
 #ifdef CONFIG_BLK_DEV_ZONED




_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-pci: prevent SK Hynix PE8000 from using Write Zeroes command

[Christoph Hellwig]

On Wed, Feb 10, 2021 at 10:11:15PM +0000, Chaitanya Kulkarni wrote:
> On 2/10/21 5:20 AM, Christoph Hellwig wrote:
> > MDTS vs Write Zeroes is a common misconception.  We had that discussion
> > in the NVMe working group, and while the text should pretty clear that
> > MDTS only applies to data transfers many implementators did not understand
> > that, which is why we added the clarification you quoted.
> >
> > Also the next NVMe spec will allow devices to advertise an explicit
> > Write Zeroes limit, take a look at TP4040 from the "NVM Express 1.4 Ratified
> > TPs" at https://nvmexpress.org/developers/nvme-specification/.
> >
> > So I think limiting to MDTS unless the new WZSL is set might be a fail
> > safe option, even if it is more pessimistic than what the spec says.
> >
> > I'd also love to retest most Write Zeroes quirks with that in place.
> 
> Do you prefer some variant of the following patch (totally untested)? OR
> something else ?

Somwhat.  As said I suspect defaulting to MDTS with a big fat comment
might make most sense unless the new WZSL is set.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-pci: prevent SK Hynix PE8000 from using Write Zeroes command

[Chaitanya Kulkarni]

On 2/10/21 11:07 PM, Christoph Hellwig wrote:
>>> I'd also love to retest most Write Zeroes quirks with that in place.
>> Do you prefer some variant of the following patch (totally untested)? OR
>> something else ?
> Somwhat.  As said I suspect defaulting to MDTS with a big fat comment
> might make most sense unless the new WZSL is set.
>
Okay, will send out patche(s) soon.

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-pci: prevent SK Hynix PE8000 from using Write Zeroes command

[Christoph Hellwig]

On Thu, Feb 11, 2021 at 06:11:19PM +0000, Chaitanya Kulkarni wrote:
> On 2/10/21 11:07 PM, Christoph Hellwig wrote:
> >>> I'd also love to retest most Write Zeroes quirks with that in place.
> >> Do you prefer some variant of the following patch (totally untested)? OR
> >> something else ?
> > Somwhat.  As said I suspect defaulting to MDTS with a big fat comment
> > might make most sense unless the new WZSL is set.
> >
> Okay, will send out patche(s) soon.

Did you get to this?  Or did I just miss it?

_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-pci: prevent SK Hynix PE8000 from using Write Zeroes command

[Chaitanya Kulkarni]

On 2/24/21 01:13, Christoph Hellwig wrote:
> On Thu, Feb 11, 2021 at 06:11:19PM +0000, Chaitanya Kulkarni wrote:
>> On 2/10/21 11:07 PM, Christoph Hellwig wrote:
>>>>> I'd also love to retest most Write Zeroes quirks with that in place.
>>>> Do you prefer some variant of the following patch (totally untested)? OR
>>>> something else ?
>>> Somwhat.  As said I suspect defaulting to MDTS with a big fat comment
>>> might make most sense unless the new WZSL is set.
>>>
>> Okay, will send out patche(s) soon.
> Did you get to this?  Or did I just miss it?
>
I sent out WIP to Gopal on 02/13 due to lack of drive.

Not sure if makes sense to start the review where patch is
not tested on the real H/w, here it is :-

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index e68a8c4ac5a6..62c928fb0ded 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1965,6 +1965,7 @@ static void nvme_config_discard(struct gendisk
*disk, struct nvme_ns *ns)
 
 static void nvme_config_write_zeroes(struct gendisk *disk, struct
nvme_ns *ns)
 {
+       bool use_mdts = ns->ctrl->quirks & NVME_QUIRK_USE_MDTS_WRITE_ZEROES;
        u64 max_blocks;
 
        if (!(ns->ctrl->oncs & NVME_CTRL_ONCS_WRITE_ZEROES) ||
@@ -1980,7 +1981,9 @@ static void nvme_config_write_zeroes(struct
gendisk *disk, struct nvme_ns *ns)
         * configured based on the controller's MDTS field in the
         * nvme_init_identify() if available.
         */
-       if (ns->ctrl->max_hw_sectors == UINT_MAX)
+       if (use_mdts && ns->ctrl->max_mdts_hw_sectors)
+               max_blocks = ns->ctrl->max_mdts_hw_sectors + 1;
+       else if (ns->ctrl->max_hw_sectors == UINT_MAX)
                max_blocks = (u64)USHRT_MAX + 1;
        else
                max_blocks = ns->ctrl->max_hw_sectors + 1;
@@ -3136,10 +3139,10 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 
        atomic_set(&ctrl->abort_limit, id->acl + 1);
        ctrl->vwc = id->vwc;
+       ctrl->max_mdts_hw_sectors = 0;
+       max_hw_sectors = UINT_MAX;
        if (id->mdts)
                max_hw_sectors = 1 << (id->mdts + page_shift - 9);
-       else
-               max_hw_sectors = UINT_MAX;
        ctrl->max_hw_sectors =
                min_not_zero(ctrl->max_hw_sectors, max_hw_sectors);
 
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 07b34175c6ce..906b26b3d306 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -150,6 +150,12 @@ enum nvme_quirks {
         * 48 bits.
         */
        NVME_QUIRK_DMA_ADDRESS_BITS_48          = (1 << 16),
+
+       /*
+        * The controller doesn't allow the write zeores command of size
+        * more than reported by the MDTS
+        */
+       NVME_QUIRK_USE_MDTS_WRITE_ZEROES        = (1 << 17)
 };
 
 /*
@@ -274,6 +280,7 @@ struct nvme_ctrl {
 
        u64 cap;
        u32 max_hw_sectors;
+       u32 max_mdts_hw_sectors;
        u32 max_segments;
        u32 max_integrity_segments;
 #ifdef CONFIG_BLK_DEV_ZONED
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 207137a0ed8e..f9f36d26828b 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -3268,6 +3268,8 @@ static const struct pci_device_id nvme_id_table[] = {
                .driver_data = NVME_QUIRK_DISABLE_WRITE_ZEROES, },
        { PCI_DEVICE(0x1d97, 0x2263),   /* SPCC */
                .driver_data = NVME_QUIRK_DISABLE_WRITE_ZEROES, },
+       { PCI_DEVICE(0x1c5c, 0x2839),  /* SK Hynix PE8000 U.3 NVMe
storage */
+               .driver_data = NVME_QUIRK_USE_MDTS_WRITE_ZEROES, },
        { PCI_DEVICE(0x2646, 0x2262),   /* KINGSTON SKC2000 NVMe SSD */
                .driver_data = NVME_QUIRK_NO_DEEPEST_PS, },
        { PCI_DEVICE(0x2646, 0x2263),   /* KINGSTON A2000 NVMe SSD  */
diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index cb6f86572b24..38fffee6b85e 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -688,7 +688,8 @@ static struct nvmf_transport_ops nvme_loop_transport = {
        .name           = "loop",
        .module         = THIS_MODULE,
        .create_ctrl    = nvme_loop_create_ctrl,
-       .allowed_opts   = NVMF_OPT_TRADDR,
+       .allowed_opts   = NVMF_OPT_TRADDR | NVMF_OPT_CTRL_LOSS_TMO,
+
 };
 
 static int __init nvme_loop_init_module(void)


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme

Re: [PATCH] nvme-pci: prevent SK Hynix PE8000 from using Write Zeroes command

[Gopal Tiwari]

Hi Chaitanya, 

No feedback on the patch yet. 

Thanks 
Gopal  

----- Original Message -----
From: "Chaitanya Kulkarni" <Chaitanya.Kulkarni@wdc.com>
To: "Christoph Hellwig" <hch@lst.de>
Cc: hch@infradead.org, kbusch@kernel.org, "Gopal Tiwari" <gtiwari@redhat.com>, linux-nvme@lists.infradead.org
Sent: Thursday, February 25, 2021 7:39:33 AM
Subject: Re: [PATCH] nvme-pci: prevent SK Hynix PE8000 from using Write Zeroes command

On 2/24/21 01:13, Christoph Hellwig wrote:
> On Thu, Feb 11, 2021 at 06:11:19PM +0000, Chaitanya Kulkarni wrote:
>> On 2/10/21 11:07 PM, Christoph Hellwig wrote:
>>>>> I'd also love to retest most Write Zeroes quirks with that in place.
>>>> Do you prefer some variant of the following patch (totally untested)? OR
>>>> something else ?
>>> Somwhat.  As said I suspect defaulting to MDTS with a big fat comment
>>> might make most sense unless the new WZSL is set.
>>>
>> Okay, will send out patche(s) soon.
> Did you get to this?  Or did I just miss it?
>
I sent out WIP to Gopal on 02/13 due to lack of drive.

Not sure if makes sense to start the review where patch is
not tested on the real H/w, here it is :-

diff --git a/drivers/nvme/host/core.c b/drivers/nvme/host/core.c
index e68a8c4ac5a6..62c928fb0ded 100644
--- a/drivers/nvme/host/core.c
+++ b/drivers/nvme/host/core.c
@@ -1965,6 +1965,7 @@ static void nvme_config_discard(struct gendisk
*disk, struct nvme_ns *ns)
 
 static void nvme_config_write_zeroes(struct gendisk *disk, struct
nvme_ns *ns)
 {
+       bool use_mdts = ns->ctrl->quirks & NVME_QUIRK_USE_MDTS_WRITE_ZEROES;
        u64 max_blocks;
 
        if (!(ns->ctrl->oncs & NVME_CTRL_ONCS_WRITE_ZEROES) ||
@@ -1980,7 +1981,9 @@ static void nvme_config_write_zeroes(struct
gendisk *disk, struct nvme_ns *ns)
         * configured based on the controller's MDTS field in the
         * nvme_init_identify() if available.
         */
-       if (ns->ctrl->max_hw_sectors == UINT_MAX)
+       if (use_mdts && ns->ctrl->max_mdts_hw_sectors)
+               max_blocks = ns->ctrl->max_mdts_hw_sectors + 1;
+       else if (ns->ctrl->max_hw_sectors == UINT_MAX)
                max_blocks = (u64)USHRT_MAX + 1;
        else
                max_blocks = ns->ctrl->max_hw_sectors + 1;
@@ -3136,10 +3139,10 @@ int nvme_init_identify(struct nvme_ctrl *ctrl)
 
        atomic_set(&ctrl->abort_limit, id->acl + 1);
        ctrl->vwc = id->vwc;
+       ctrl->max_mdts_hw_sectors = 0;
+       max_hw_sectors = UINT_MAX;
        if (id->mdts)
                max_hw_sectors = 1 << (id->mdts + page_shift - 9);
-       else
-               max_hw_sectors = UINT_MAX;
        ctrl->max_hw_sectors =
                min_not_zero(ctrl->max_hw_sectors, max_hw_sectors);
 
diff --git a/drivers/nvme/host/nvme.h b/drivers/nvme/host/nvme.h
index 07b34175c6ce..906b26b3d306 100644
--- a/drivers/nvme/host/nvme.h
+++ b/drivers/nvme/host/nvme.h
@@ -150,6 +150,12 @@ enum nvme_quirks {
         * 48 bits.
         */
        NVME_QUIRK_DMA_ADDRESS_BITS_48          = (1 << 16),
+
+       /*
+        * The controller doesn't allow the write zeores command of size
+        * more than reported by the MDTS
+        */
+       NVME_QUIRK_USE_MDTS_WRITE_ZEROES        = (1 << 17)
 };
 
 /*
@@ -274,6 +280,7 @@ struct nvme_ctrl {
 
        u64 cap;
        u32 max_hw_sectors;
+       u32 max_mdts_hw_sectors;
        u32 max_segments;
        u32 max_integrity_segments;
 #ifdef CONFIG_BLK_DEV_ZONED
diff --git a/drivers/nvme/host/pci.c b/drivers/nvme/host/pci.c
index 207137a0ed8e..f9f36d26828b 100644
--- a/drivers/nvme/host/pci.c
+++ b/drivers/nvme/host/pci.c
@@ -3268,6 +3268,8 @@ static const struct pci_device_id nvme_id_table[] = {
                .driver_data = NVME_QUIRK_DISABLE_WRITE_ZEROES, },
        { PCI_DEVICE(0x1d97, 0x2263),   /* SPCC */
                .driver_data = NVME_QUIRK_DISABLE_WRITE_ZEROES, },
+       { PCI_DEVICE(0x1c5c, 0x2839),  /* SK Hynix PE8000 U.3 NVMe
storage */
+               .driver_data = NVME_QUIRK_USE_MDTS_WRITE_ZEROES, },
        { PCI_DEVICE(0x2646, 0x2262),   /* KINGSTON SKC2000 NVMe SSD */
                .driver_data = NVME_QUIRK_NO_DEEPEST_PS, },
        { PCI_DEVICE(0x2646, 0x2263),   /* KINGSTON A2000 NVMe SSD  */
diff --git a/drivers/nvme/target/loop.c b/drivers/nvme/target/loop.c
index cb6f86572b24..38fffee6b85e 100644
--- a/drivers/nvme/target/loop.c
+++ b/drivers/nvme/target/loop.c
@@ -688,7 +688,8 @@ static struct nvmf_transport_ops nvme_loop_transport = {
        .name           = "loop",
        .module         = THIS_MODULE,
        .create_ctrl    = nvme_loop_create_ctrl,
-       .allowed_opts   = NVMF_OPT_TRADDR,
+       .allowed_opts   = NVMF_OPT_TRADDR | NVMF_OPT_CTRL_LOSS_TMO,
+
 };
 
 static int __init nvme_loop_init_module(void)


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme


_______________________________________________
Linux-nvme mailing list
Linux-nvme@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-nvme