[RFC PATCH 0/2] block: Remove skd driver

Hi Jens,

Instead of spending time fixing the skd driver to (at the very least)
fix the call to set_capacity() with IRQ disabled, I am proposing to
simply remove this driver. The STEC S1220 cards are EOL since 2014 and
not supported by the vendor since several years ago. Given that these
SSDs are very slow by today's NVMe standard, I do not think it is
worthwhile to maintain this driver with newer kernel versions. I will
keep addressing any problem that shows up with LTS versions.

The first patch removes the skd driver and the second patch reverts
commit 0fe37724f8e7 ("block: fix bd_size_lock use") as the skd driver
was the one driver that needed this (not so nice) fix.

Please let me know what you think about this.

Damien Le Moal (2):
  block: remove skd driver
  block: revert "block: fix bd_size_lock use"

 MAINTAINERS               |    6 -
 block/genhd.c             |    5 +-
 block/partitions/core.c   |    6 +-
 drivers/block/Kconfig     |   10 -
 drivers/block/Makefile    |    2 -
 drivers/block/skd_main.c  | 3670 -------------------------------------
 drivers/block/skd_s1120.h |  322 ----
 7 files changed, 4 insertions(+), 4017 deletions(-)
 delete mode 100644 drivers/block/skd_main.c
 delete mode 100644 drivers/block/skd_s1120.h

-- 
2.29.2

With the removal of the skd driver, using IRQ safe locking of a bdev
bd_size_lock spinlock to protect the bdev inode size is not necessary
anymore as there is no other known driver using this lock under an IRQ
disabled context (e.g. calling set_capacity() with IRQ disabled).
Revert commit 0fe37724f8e7 ("block: fix bd_size_lock use") which
introduced the IRQ safe change.

Signed-off-by: Damien Le Moal <redacted>
---
 block/genhd.c           | 5 ++---
 block/partitions/core.c | 6 ++----
 2 files changed, 4 insertions(+), 7 deletions(-)

diff --git a/block/genhd.c b/block/genhd.c
index 9e741a4f351b..419548e92d82 100644
--- a/block/genhd.c
+++ b/block/genhd.c

@@ -45,11 +45,10 @@ static void disk_release_events(struct gendisk *disk);
 void set_capacity(struct gendisk *disk, sector_t sectors)
 {
 	struct block_device *bdev = disk->part0;
-	unsigned long flags;
 
-	spin_lock_irqsave(&bdev->bd_size_lock, flags);
+	spin_lock(&bdev->bd_size_lock);
 	i_size_write(bdev->bd_inode, (loff_t)sectors << SECTOR_SHIFT);
-	spin_unlock_irqrestore(&bdev->bd_size_lock, flags);
+	spin_unlock(&bdev->bd_size_lock);
 }
 EXPORT_SYMBOL(set_capacity);
 

diff --git a/block/partitions/core.c b/block/partitions/core.c
index 081f1df9d10d..d0745555ba16 100644
--- a/block/partitions/core.c
+++ b/block/partitions/core.c

@@ -88,11 +88,9 @@ static int (*check_part[])(struct parsed_partitions *) = {
 
 static void bdev_set_nr_sectors(struct block_device *bdev, sector_t sectors)
 {
-	unsigned long flags;
-
-	spin_lock_irqsave(&bdev->bd_size_lock, flags);
+	spin_lock(&bdev->bd_size_lock);
 	i_size_write(bdev->bd_inode, (loff_t)sectors << SECTOR_SHIFT);
-	spin_unlock_irqrestore(&bdev->bd_size_lock, flags);
+	spin_unlock(&bdev->bd_size_lock);
 }
 
 static struct parsed_partitions *allocate_partitions(struct gendisk *hd)

-- 
2.29.2

On 2021/02/04 18:01, Johannes Thumshirn wrote:

Given that #1 in this series is accepted,
Reviewed-by: Johannes Thumshirn <redacted>

Do you mean "if #1 in this series is accepted" ?
I have not received patch #1... I wonder if it hit the list ? Did you get it ?

-- 
Damien Le Moal
Western Digital Research

Given that #1 in this series is accepted,
Reviewed-by: Johannes Thumshirn <redacted>

On 04/02/2021 10:03, Damien Le Moal wrote:

On 2021/02/04 18:01, Johannes Thumshirn wrote:

quoted

Given that #1 in this series is accepted,
Reviewed-by: Johannes Thumshirn <redacted>

Do you mean "if #1 in this series is accepted" ?
I have not received patch #1... I wonder if it hit the list ? Did you get it ?

Me neither, seems like vger is acting funny again

On 2021/02/04 18:14, Johannes Thumshirn wrote:

On 04/02/2021 10:03, Damien Le Moal wrote:

quoted

On 2021/02/04 18:01, Johannes Thumshirn wrote:

quoted

Given that #1 in this series is accepted,
Reviewed-by: Johannes Thumshirn <redacted>

Do you mean "if #1 in this series is accepted" ?
I have not received patch #1... I wonder if it hit the list ? Did you get it ?

Me neither, seems like vger is acting funny again

Or Exchange is again acting up. It does not show up in lore... Resending.

-- 
Damien Le Moal
Western Digital Research

On 2021/02/04 18:14, Johannes Thumshirn wrote:

On 04/02/2021 10:03, Damien Le Moal wrote:

quoted

On 2021/02/04 18:01, Johannes Thumshirn wrote:

quoted

Given that #1 in this series is accepted,
Reviewed-by: Johannes Thumshirn <redacted>

Do you mean "if #1 in this series is accepted" ?
I have not received patch #1... I wonder if it hit the list ? Did you get it ?

Me neither, seems like vger is acting funny again

Patch 1 is 111KB... Probably too big and it gets dropped by vger.

I generated it with "git rm" and a few other changes (MAITAINERS, Kconfig and
Makefile). Is it possible to shorten the "git rm" part so that it does not show
every single line removed but just the files being deleted ? That driver is a
single file, so very large.


-- 
Damien Le Moal
Western Digital Research

On 04/02/2021 10:20, Damien Le Moal wrote:

On 2021/02/04 18:14, Johannes Thumshirn wrote:

quoted

On 04/02/2021 10:03, Damien Le Moal wrote:

quoted

On 2021/02/04 18:01, Johannes Thumshirn wrote:

quoted

Given that #1 in this series is accepted,
Reviewed-by: Johannes Thumshirn <redacted>

Do you mean "if #1 in this series is accepted" ?
I have not received patch #1... I wonder if it hit the list ? Did you get it ?

Me neither, seems like vger is acting funny again

Patch 1 is 111KB... Probably too big and it gets dropped by vger.

I generated it with "git rm" and a few other changes (MAITAINERS, Kconfig and
Makefile). Is it possible to shorten the "git rm" part so that it does not show
every single line removed but just the files being deleted ? That driver is a
single file, so very large.

Well that's bad, but how could you remove the driver then if vger doesn't  like it?

On 2021/02/04 18:25, Johannes Thumshirn wrote:

On 04/02/2021 10:20, Damien Le Moal wrote:

quoted

On 2021/02/04 18:14, Johannes Thumshirn wrote:

quoted

On 04/02/2021 10:03, Damien Le Moal wrote:

quoted

On 2021/02/04 18:01, Johannes Thumshirn wrote:

quoted

Given that #1 in this series is accepted,
Reviewed-by: Johannes Thumshirn <redacted>

Do you mean "if #1 in this series is accepted" ?
I have not received patch #1... I wonder if it hit the list ? Did you get it ?

Me neither, seems like vger is acting funny again

Patch 1 is 111KB... Probably too big and it gets dropped by vger.

I generated it with "git rm" and a few other changes (MAITAINERS, Kconfig and
Makefile). Is it possible to shorten the "git rm" part so that it does not show
every single line removed but just the files being deleted ? That driver is a
single file, so very large.

Well that's bad, but how could you remove the driver then if vger doesn't  like it?

No clue. Just hope Jens got the email.

-- 
Damien Le Moal
Western Digital Research

On 2/4/21 1:43 AM, Damien Le Moal wrote:

Hi Jens,

Instead of spending time fixing the skd driver to (at the very least)
fix the call to set_capacity() with IRQ disabled, I am proposing to
simply remove this driver. The STEC S1220 cards are EOL since 2014 and
not supported by the vendor since several years ago. Given that these
SSDs are very slow by today's NVMe standard, I do not think it is
worthwhile to maintain this driver with newer kernel versions. I will
keep addressing any problem that shows up with LTS versions.

The first patch removes the skd driver and the second patch reverts
commit 0fe37724f8e7 ("block: fix bd_size_lock use") as the skd driver
was the one driver that needed this (not so nice) fix.

Please let me know what you think about this.

I'm fine with removing it. The 5.12 branch doesn't have the later
fix for the bd_size_lock issue, so could you just resend that once
the merge window opens and the block bits have gone in? In case I
forget...

-- 
Jens Axboe

On 2/4/21 7:52 AM, Damien Le Moal wrote:

On 2021/02/04 23:46, Jens Axboe wrote:

quoted

On 2/4/21 1:43 AM, Damien Le Moal wrote:

quoted

Hi Jens,

Instead of spending time fixing the skd driver to (at the very least)
fix the call to set_capacity() with IRQ disabled, I am proposing to
simply remove this driver. The STEC S1220 cards are EOL since 2014 and
not supported by the vendor since several years ago. Given that these
SSDs are very slow by today's NVMe standard, I do not think it is
worthwhile to maintain this driver with newer kernel versions. I will
keep addressing any problem that shows up with LTS versions.

The first patch removes the skd driver and the second patch reverts
commit 0fe37724f8e7 ("block: fix bd_size_lock use") as the skd driver
was the one driver that needed this (not so nice) fix.

Please let me know what you think about this.

I'm fine with removing it. The 5.12 branch doesn't have the later
fix for the bd_size_lock issue, so could you just resend that once
the merge window opens and the block bits have gone in? In case I
forget...

OK. Will do.

Could you confirm if you received patch #1 ? It looks like the list server is
dropping it likely because it is too big.

The list is a huge mess these days, including lore. So not sure what is
going on. I did receive it, but it wasn't on lore, hence probably only
because I was CC'ed on it.

-- 
Jens Axboe

On 2021/02/04 23:55, Jens Axboe wrote:

On 2/4/21 7:52 AM, Damien Le Moal wrote:

quoted

On 2021/02/04 23:46, Jens Axboe wrote:

quoted

On 2/4/21 1:43 AM, Damien Le Moal wrote:

quoted

Hi Jens,

Instead of spending time fixing the skd driver to (at the very least)
fix the call to set_capacity() with IRQ disabled, I am proposing to
simply remove this driver. The STEC S1220 cards are EOL since 2014 and
not supported by the vendor since several years ago. Given that these
SSDs are very slow by today's NVMe standard, I do not think it is
worthwhile to maintain this driver with newer kernel versions. I will
keep addressing any problem that shows up with LTS versions.

The first patch removes the skd driver and the second patch reverts
commit 0fe37724f8e7 ("block: fix bd_size_lock use") as the skd driver
was the one driver that needed this (not so nice) fix.

Please let me know what you think about this.

I'm fine with removing it. The 5.12 branch doesn't have the later
fix for the bd_size_lock issue, so could you just resend that once
the merge window opens and the block bits have gone in? In case I
forget...

OK. Will do.

Could you confirm if you received patch #1 ? It looks like the list server is
dropping it likely because it is too big.

The list is a huge mess these days, including lore. So not sure what is
going on. I did receive it, but it wasn't on lore, hence probably only
because I was CC'ed on it.

Thanks for the confirmation. I will make sure to keep you in cc.


-- 
Damien Le Moal
Western Digital Research

On 2021/02/04 23:46, Jens Axboe wrote:

On 2/4/21 1:43 AM, Damien Le Moal wrote:

quoted

Hi Jens,

Instead of spending time fixing the skd driver to (at the very least)
fix the call to set_capacity() with IRQ disabled, I am proposing to
simply remove this driver. The STEC S1220 cards are EOL since 2014 and
not supported by the vendor since several years ago. Given that these
SSDs are very slow by today's NVMe standard, I do not think it is
worthwhile to maintain this driver with newer kernel versions. I will
keep addressing any problem that shows up with LTS versions.

The first patch removes the skd driver and the second patch reverts
commit 0fe37724f8e7 ("block: fix bd_size_lock use") as the skd driver
was the one driver that needed this (not so nice) fix.

Please let me know what you think about this.

I'm fine with removing it. The 5.12 branch doesn't have the later
fix for the bd_size_lock issue, so could you just resend that once
the merge window opens and the block bits have gone in? In case I
forget...

OK. Will do.

Could you confirm if you received patch #1 ? It looks like the list server is
dropping it likely because it is too big.

-- 
Damien Le Moal
Western Digital Research