* [PATCH 1/9] pstore/zone: cap the maximum device size
2020-10-16 13:20 simplify pstore-blk Christoph Hellwig
@ 2020-10-16 13:20 ` Christoph Hellwig
2020-11-08 14:05 ` 廖威雄
` (2 more replies)
2020-10-16 13:20 ` [PATCH 2/9] pstore/blk: update the command line example Christoph Hellwig
` (9 subsequent siblings)
10 siblings, 3 replies; 28+ messages in thread
From: Christoph Hellwig @ 2020-10-16 13:20 UTC (permalink / raw)
To: Kees Cook, Anton Vorontsov, Colin Cross, Tony Luck, WeiXiong Liao
Cc: linux-mtd, linux-kernel
Introduce an abritrary 128MiB cap to avoid malloc failures when using
a larger block device.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/pstore/zone.c | 4 ++++
1 file changed, 4 insertions(+)
diff --git a/fs/pstore/zone.c b/fs/pstore/zone.c
index 3ce89216670c9b..5266ccbec007f3 100644
--- a/fs/pstore/zone.c
+++ b/fs/pstore/zone.c
@@ -1299,6 +1299,10 @@ int register_pstore_zone(struct pstore_zone_info *info)
pr_warn("total_size must be >= 4096\n");
return -EINVAL;
}
+ if (info->total_size > SZ_128M) {
+ pr_warn("capping size to 128MiB\n");
+ info->total_size = SZ_128M;
+ }
if (!info->kmsg_size && !info->pmsg_size && !info->console_size &&
!info->ftrace_size) {
--
2.28.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 1/9] pstore/zone: cap the maximum device size
2020-10-16 13:20 ` [PATCH 1/9] pstore/zone: cap the maximum device size Christoph Hellwig
@ 2020-11-08 14:05 ` 廖威雄
2020-12-01 19:08 ` Kees Cook
2020-12-01 21:11 ` Kees Cook
2 siblings, 0 replies; 28+ messages in thread
From: 廖威雄 @ 2020-11-08 14:05 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Kees Cook, Anton Vorontsov, Colin Cross, Tony Luck,
WeiXiong Liao, linux-mtd, linux-kernel
On Fri, Oct 16, 2020 at 9:27 PM Christoph Hellwig <hch@lst.de> wrote:
>
> Introduce an abritrary 128MiB cap to avoid malloc failures when using
> a larger block device.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/pstore/zone.c | 4 ++++
> 1 file changed, 4 insertions(+)
>
> diff --git a/fs/pstore/zone.c b/fs/pstore/zone.c
> index 3ce89216670c9b..5266ccbec007f3 100644
> --- a/fs/pstore/zone.c
> +++ b/fs/pstore/zone.c
> @@ -1299,6 +1299,10 @@ int register_pstore_zone(struct pstore_zone_info *info)
> pr_warn("total_size must be >= 4096\n");
> return -EINVAL;
> }
> + if (info->total_size > SZ_128M) {
> + pr_warn("capping size to 128MiB\n");
> + info->total_size = SZ_128M;
> + }
>
Reviewed-by: WeiXiong Liao <gmpy.liaowx@gmail.com>
> if (!info->kmsg_size && !info->pmsg_size && !info->console_size &&
> !info->ftrace_size) {
> --
> 2.28.0
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/9] pstore/zone: cap the maximum device size
2020-10-16 13:20 ` [PATCH 1/9] pstore/zone: cap the maximum device size Christoph Hellwig
2020-11-08 14:05 ` 廖威雄
@ 2020-12-01 19:08 ` Kees Cook
2020-12-01 21:11 ` Kees Cook
2 siblings, 0 replies; 28+ messages in thread
From: Kees Cook @ 2020-12-01 19:08 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Anton Vorontsov, Colin Cross, Tony Luck, WeiXiong Liao,
linux-mtd, linux-kernel
On Fri, Oct 16, 2020 at 03:20:39PM +0200, Christoph Hellwig wrote:
> Introduce an abritrary 128MiB cap to avoid malloc failures when using
> a larger block device.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Yeah, this seems reasonable. I'm always worried about arbitrary size
limits, but if 128M is needed for pstore, something is likely very very
wrong. :)
Reviewed-by: Kees Cook <keescook@chromium.org>
--
Kees Cook
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 1/9] pstore/zone: cap the maximum device size
2020-10-16 13:20 ` [PATCH 1/9] pstore/zone: cap the maximum device size Christoph Hellwig
2020-11-08 14:05 ` 廖威雄
2020-12-01 19:08 ` Kees Cook
@ 2020-12-01 21:11 ` Kees Cook
2 siblings, 0 replies; 28+ messages in thread
From: Kees Cook @ 2020-12-01 21:11 UTC (permalink / raw)
To: WeiXiong Liao, Christoph Hellwig, Colin Cross, Anton Vorontsov,
Tony Luck
Cc: Kees Cook, linux-kernel, linux-mtd
On Fri, 16 Oct 2020 15:20:39 +0200, Christoph Hellwig wrote:
> Introduce an abritrary 128MiB cap to avoid malloc failures when using
> a larger block device.
I've applied 1-3 of the series to for-next/pstore, thanks!
[1/9] pstore/zone: cap the maximum device size
https://git.kernel.org/kees/c/cbf82e35031b
[2/9] pstore/blk: update the command line example
https://git.kernel.org/kees/c/45a8af4412b1
[3/9] pstore/blk: remove {un,}register_pstore_blk
https://git.kernel.org/kees/c/b6f8ed33ab2b
(I tweaked #3 with a small added comment.)
--
Kees Cook
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 2/9] pstore/blk: update the command line example
2020-10-16 13:20 simplify pstore-blk Christoph Hellwig
2020-10-16 13:20 ` [PATCH 1/9] pstore/zone: cap the maximum device size Christoph Hellwig
@ 2020-10-16 13:20 ` Christoph Hellwig
2020-11-08 14:05 ` 廖威雄
2020-12-01 19:10 ` Kees Cook
2020-10-16 13:20 ` [PATCH 3/9] pstore/blk: remove {un,}register_pstore_blk Christoph Hellwig
` (8 subsequent siblings)
10 siblings, 2 replies; 28+ messages in thread
From: Christoph Hellwig @ 2020-10-16 13:20 UTC (permalink / raw)
To: Kees Cook, Anton Vorontsov, Colin Cross, Tony Luck, WeiXiong Liao
Cc: linux-mtd, linux-kernel
Use the human readable device name instead of the device number, and
add the required best_effort parameter.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
Documentation/admin-guide/pstore-blk.rst | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/Documentation/admin-guide/pstore-blk.rst b/Documentation/admin-guide/pstore-blk.rst
index 296d5027787ac2..d9ec8b0572d3b2 100644
--- a/Documentation/admin-guide/pstore-blk.rst
+++ b/Documentation/admin-guide/pstore-blk.rst
@@ -35,7 +35,7 @@ module parameters have priority over Kconfig.
Here is an example for module parameters::
- pstore_blk.blkdev=179:7 pstore_blk.kmsg_size=64
+ pstore_blk.blkdev=/dev/mmcblk0p7 pstore_blk.kmsg_size=64 best_effort=y
The detail of each configurations may be of interest to you.
--
2.28.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 2/9] pstore/blk: update the command line example
2020-10-16 13:20 ` [PATCH 2/9] pstore/blk: update the command line example Christoph Hellwig
@ 2020-11-08 14:05 ` 廖威雄
2020-12-01 19:10 ` Kees Cook
1 sibling, 0 replies; 28+ messages in thread
From: 廖威雄 @ 2020-11-08 14:05 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Kees Cook, Anton Vorontsov, Colin Cross, Tony Luck,
WeiXiong Liao, linux-mtd, linux-kernel
On Fri, Oct 16, 2020 at 9:28 PM Christoph Hellwig <hch@lst.de> wrote:
>
> Use the human readable device name instead of the device number, and
> add the required best_effort parameter.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> Documentation/admin-guide/pstore-blk.rst | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/admin-guide/pstore-blk.rst b/Documentation/admin-guide/pstore-blk.rst
> index 296d5027787ac2..d9ec8b0572d3b2 100644
> --- a/Documentation/admin-guide/pstore-blk.rst
> +++ b/Documentation/admin-guide/pstore-blk.rst
> @@ -35,7 +35,7 @@ module parameters have priority over Kconfig.
>
> Here is an example for module parameters::
>
> - pstore_blk.blkdev=179:7 pstore_blk.kmsg_size=64
> + pstore_blk.blkdev=/dev/mmcblk0p7 pstore_blk.kmsg_size=64 best_effort=y
>
Reviewed-by: WeiXiong Liao <gmpy.liaowx@gmail.com>
> The detail of each configurations may be of interest to you.
>
> --
> 2.28.0
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 2/9] pstore/blk: update the command line example
2020-10-16 13:20 ` [PATCH 2/9] pstore/blk: update the command line example Christoph Hellwig
2020-11-08 14:05 ` 廖威雄
@ 2020-12-01 19:10 ` Kees Cook
1 sibling, 0 replies; 28+ messages in thread
From: Kees Cook @ 2020-12-01 19:10 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Anton Vorontsov, Colin Cross, Tony Luck, WeiXiong Liao,
linux-mtd, linux-kernel
On Fri, Oct 16, 2020 at 03:20:40PM +0200, Christoph Hellwig wrote:
> Use the human readable device name instead of the device number, and
> add the required best_effort parameter.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Yeah, best_effort is needed here. I think changing from major:minor to a
path is also fine.
Reviewed-by: Kees Cook <keescook@chromium.org>
> ---
> Documentation/admin-guide/pstore-blk.rst | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/admin-guide/pstore-blk.rst b/Documentation/admin-guide/pstore-blk.rst
> index 296d5027787ac2..d9ec8b0572d3b2 100644
> --- a/Documentation/admin-guide/pstore-blk.rst
> +++ b/Documentation/admin-guide/pstore-blk.rst
> @@ -35,7 +35,7 @@ module parameters have priority over Kconfig.
>
> Here is an example for module parameters::
>
> - pstore_blk.blkdev=179:7 pstore_blk.kmsg_size=64
> + pstore_blk.blkdev=/dev/mmcblk0p7 pstore_blk.kmsg_size=64 best_effort=y
>
> The detail of each configurations may be of interest to you.
>
> --
> 2.28.0
>
--
Kees Cook
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 3/9] pstore/blk: remove {un,}register_pstore_blk
2020-10-16 13:20 simplify pstore-blk Christoph Hellwig
2020-10-16 13:20 ` [PATCH 1/9] pstore/zone: cap the maximum device size Christoph Hellwig
2020-10-16 13:20 ` [PATCH 2/9] pstore/blk: update the command line example Christoph Hellwig
@ 2020-10-16 13:20 ` Christoph Hellwig
2020-12-01 19:29 ` Kees Cook
2020-10-16 13:20 ` [PATCH 4/9] pstore/blk: remove __unregister_pstore_blk Christoph Hellwig
` (7 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Christoph Hellwig @ 2020-10-16 13:20 UTC (permalink / raw)
To: Kees Cook, Anton Vorontsov, Colin Cross, Tony Luck, WeiXiong Liao
Cc: linux-mtd, linux-kernel
This interface is entirely unused, so remove them and various bits of
unreachable code.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
Documentation/admin-guide/pstore-blk.rst | 8 +--
fs/pstore/blk.c | 79 ++----------------------
include/linux/pstore_blk.h | 42 -------------
3 files changed, 7 insertions(+), 122 deletions(-)
diff --git a/Documentation/admin-guide/pstore-blk.rst b/Documentation/admin-guide/pstore-blk.rst
index d9ec8b0572d3b2..84477621384d85 100644
--- a/Documentation/admin-guide/pstore-blk.rst
+++ b/Documentation/admin-guide/pstore-blk.rst
@@ -151,13 +151,7 @@ otherwise KMSG_DUMP_MAX.
Configurations for driver
-------------------------
-Only a block device driver cares about these configurations. A block device
-driver uses ``register_pstore_blk`` to register to pstore/blk.
-
-.. kernel-doc:: fs/pstore/blk.c
- :identifiers: register_pstore_blk
-
-A non-block device driver uses ``register_pstore_device`` with
+A device driver uses ``register_pstore_device`` with
``struct pstore_device_info`` to register to pstore/blk.
.. kernel-doc:: fs/pstore/blk.c
diff --git a/fs/pstore/blk.c b/fs/pstore/blk.c
index fcd5563dde063c..7f8368e94b3604 100644
--- a/fs/pstore/blk.c
+++ b/fs/pstore/blk.c
@@ -90,7 +90,6 @@ MODULE_PARM_DESC(blkdev, "block device for pstore storage");
static DEFINE_MUTEX(pstore_blk_lock);
static struct block_device *psblk_bdev;
static struct pstore_zone_info *pstore_zone_info;
-static pstore_blk_panic_write_op blkdev_panic_write;
struct bdev_info {
dev_t devt;
@@ -341,24 +340,7 @@ static ssize_t psblk_generic_blk_write(const char *buf, size_t bytes,
return ret;
}
-static ssize_t psblk_blk_panic_write(const char *buf, size_t size,
- loff_t off)
-{
- int ret;
-
- if (!blkdev_panic_write)
- return -EOPNOTSUPP;
-
- /* size and off must align to SECTOR_SIZE for block device */
- ret = blkdev_panic_write(buf, off >> SECTOR_SHIFT,
- size >> SECTOR_SHIFT);
- /* try next zone */
- if (ret == -ENOMSG)
- return ret;
- return ret ? -EIO : size;
-}
-
-static int __register_pstore_blk(struct pstore_blk_info *info)
+static int __register_pstore_blk(void)
{
char bdev_name[BDEVNAME_SIZE];
struct block_device *bdev;
@@ -378,68 +360,34 @@ static int __register_pstore_blk(struct pstore_blk_info *info)
}
/* only allow driver matching the @blkdev */
- if (!binfo.devt || (!best_effort &&
- MAJOR(binfo.devt) != info->major)) {
- pr_debug("invalid major %u (expect %u)\n",
- info->major, MAJOR(binfo.devt));
+ if (!binfo.devt) {
+ pr_debug("no major\n");
ret = -ENODEV;
goto err_put_bdev;
}
/* psblk_bdev must be assigned before register to pstore/blk */
psblk_bdev = bdev;
- blkdev_panic_write = info->panic_write;
-
- /* Copy back block device details. */
- info->devt = binfo.devt;
- info->nr_sects = binfo.nr_sects;
- info->start_sect = binfo.start_sect;
memset(&dev, 0, sizeof(dev));
- dev.total_size = info->nr_sects << SECTOR_SHIFT;
- dev.flags = info->flags;
+ dev.total_size = binfo.nr_sects << SECTOR_SHIFT;
dev.read = psblk_generic_blk_read;
dev.write = psblk_generic_blk_write;
- dev.erase = NULL;
- dev.panic_write = info->panic_write ? psblk_blk_panic_write : NULL;
ret = __register_pstore_device(&dev);
if (ret)
goto err_put_bdev;
bdevname(bdev, bdev_name);
- pr_info("attached %s%s\n", bdev_name,
- info->panic_write ? "" : " (no dedicated panic_write!)");
+ pr_info("attached %s (no dedicated panic_write!)\n", bdev_name);
return 0;
err_put_bdev:
psblk_bdev = NULL;
- blkdev_panic_write = NULL;
psblk_put_bdev(bdev, holder);
return ret;
}
-/**
- * register_pstore_blk() - register block device to pstore/blk
- *
- * @info: details on the desired block device interface
- *
- * Return:
- * * 0 - OK
- * * Others - something error.
- */
-int register_pstore_blk(struct pstore_blk_info *info)
-{
- int ret;
-
- mutex_lock(&pstore_blk_lock);
- ret = __register_pstore_blk(info);
- mutex_unlock(&pstore_blk_lock);
-
- return ret;
-}
-EXPORT_SYMBOL_GPL(register_pstore_blk);
-
static void __unregister_pstore_blk(unsigned int major)
{
struct pstore_device_info dev = { .read = psblk_generic_blk_read };
@@ -449,24 +397,10 @@ static void __unregister_pstore_blk(unsigned int major)
if (psblk_bdev && MAJOR(psblk_bdev->bd_dev) == major) {
__unregister_pstore_device(&dev);
psblk_put_bdev(psblk_bdev, holder);
- blkdev_panic_write = NULL;
psblk_bdev = NULL;
}
}
-/**
- * unregister_pstore_blk() - unregister block device from pstore/blk
- *
- * @major: the major device number of device
- */
-void unregister_pstore_blk(unsigned int major)
-{
- mutex_lock(&pstore_blk_lock);
- __unregister_pstore_blk(major);
- mutex_unlock(&pstore_blk_lock);
-}
-EXPORT_SYMBOL_GPL(unregister_pstore_blk);
-
/* get information of pstore/blk */
int pstore_blk_get_config(struct pstore_blk_config *info)
{
@@ -483,12 +417,11 @@ EXPORT_SYMBOL_GPL(pstore_blk_get_config);
static int __init pstore_blk_init(void)
{
- struct pstore_blk_info info = { };
int ret = 0;
mutex_lock(&pstore_blk_lock);
if (!pstore_zone_info && best_effort && blkdev[0])
- ret = __register_pstore_blk(&info);
+ ret = __register_pstore_blk();
mutex_unlock(&pstore_blk_lock);
return ret;
diff --git a/include/linux/pstore_blk.h b/include/linux/pstore_blk.h
index 61e914522b0193..99564f93d77488 100644
--- a/include/linux/pstore_blk.h
+++ b/include/linux/pstore_blk.h
@@ -7,48 +7,6 @@
#include <linux/pstore.h>
#include <linux/pstore_zone.h>
-/**
- * typedef pstore_blk_panic_write_op - panic write operation to block device
- *
- * @buf: the data to write
- * @start_sect: start sector to block device
- * @sects: sectors count on buf
- *
- * Return: On success, zero should be returned. Others excluding -ENOMSG
- * mean error. -ENOMSG means to try next zone.
- *
- * Panic write to block device must be aligned to SECTOR_SIZE.
- */
-typedef int (*pstore_blk_panic_write_op)(const char *buf, sector_t start_sect,
- sector_t sects);
-
-/**
- * struct pstore_blk_info - pstore/blk registration details
- *
- * @major: Which major device number to support with pstore/blk
- * @flags: The supported PSTORE_FLAGS_* from linux/pstore.h.
- * @panic_write:The write operation only used for the panic case.
- * This can be NULL, but is recommended to avoid losing
- * crash data if the kernel's IO path or work queues are
- * broken during a panic.
- * @devt: The dev_t that pstore/blk has attached to.
- * @nr_sects: Number of sectors on @devt.
- * @start_sect: Starting sector on @devt.
- */
-struct pstore_blk_info {
- unsigned int major;
- unsigned int flags;
- pstore_blk_panic_write_op panic_write;
-
- /* Filled in by pstore/blk after registration. */
- dev_t devt;
- sector_t nr_sects;
- sector_t start_sect;
-};
-
-int register_pstore_blk(struct pstore_blk_info *info);
-void unregister_pstore_blk(unsigned int major);
-
/**
* struct pstore_device_info - back-end pstore/blk driver structure.
*
--
2.28.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 3/9] pstore/blk: remove {un,}register_pstore_blk
2020-10-16 13:20 ` [PATCH 3/9] pstore/blk: remove {un,}register_pstore_blk Christoph Hellwig
@ 2020-12-01 19:29 ` Kees Cook
0 siblings, 0 replies; 28+ messages in thread
From: Kees Cook @ 2020-12-01 19:29 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Anton Vorontsov, Colin Cross, Tony Luck, WeiXiong Liao,
linux-mtd, linux-kernel
On Fri, Oct 16, 2020 at 03:20:41PM +0200, Christoph Hellwig wrote:
> This interface is entirely unused, so remove them and various bits of
> unreachable code.
Yeah, this is fair to remove -- there are no users. I'm not a fan of
dropping it, but it can come back if anyone wants to provide a full
generic block device implementation.
Reviewed-by: Kees Cook <keescook@chromium.org>
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> Documentation/admin-guide/pstore-blk.rst | 8 +--
> fs/pstore/blk.c | 79 ++----------------------
> include/linux/pstore_blk.h | 42 -------------
> 3 files changed, 7 insertions(+), 122 deletions(-)
>
> diff --git a/Documentation/admin-guide/pstore-blk.rst b/Documentation/admin-guide/pstore-blk.rst
> index d9ec8b0572d3b2..84477621384d85 100644
> --- a/Documentation/admin-guide/pstore-blk.rst
> +++ b/Documentation/admin-guide/pstore-blk.rst
> @@ -151,13 +151,7 @@ otherwise KMSG_DUMP_MAX.
> Configurations for driver
> -------------------------
>
> -Only a block device driver cares about these configurations. A block device
> -driver uses ``register_pstore_blk`` to register to pstore/blk.
> -
> -.. kernel-doc:: fs/pstore/blk.c
> - :identifiers: register_pstore_blk
> -
> -A non-block device driver uses ``register_pstore_device`` with
> +A device driver uses ``register_pstore_device`` with
> ``struct pstore_device_info`` to register to pstore/blk.
>
> .. kernel-doc:: fs/pstore/blk.c
> diff --git a/fs/pstore/blk.c b/fs/pstore/blk.c
> index fcd5563dde063c..7f8368e94b3604 100644
> --- a/fs/pstore/blk.c
> +++ b/fs/pstore/blk.c
> @@ -90,7 +90,6 @@ MODULE_PARM_DESC(blkdev, "block device for pstore storage");
> static DEFINE_MUTEX(pstore_blk_lock);
> static struct block_device *psblk_bdev;
> static struct pstore_zone_info *pstore_zone_info;
> -static pstore_blk_panic_write_op blkdev_panic_write;
>
> struct bdev_info {
> dev_t devt;
> @@ -341,24 +340,7 @@ static ssize_t psblk_generic_blk_write(const char *buf, size_t bytes,
> return ret;
> }
>
> -static ssize_t psblk_blk_panic_write(const char *buf, size_t size,
> - loff_t off)
> -{
> - int ret;
> -
> - if (!blkdev_panic_write)
> - return -EOPNOTSUPP;
> -
> - /* size and off must align to SECTOR_SIZE for block device */
> - ret = blkdev_panic_write(buf, off >> SECTOR_SHIFT,
> - size >> SECTOR_SHIFT);
> - /* try next zone */
> - if (ret == -ENOMSG)
> - return ret;
> - return ret ? -EIO : size;
> -}
> -
> -static int __register_pstore_blk(struct pstore_blk_info *info)
> +static int __register_pstore_blk(void)
> {
> char bdev_name[BDEVNAME_SIZE];
> struct block_device *bdev;
> @@ -378,68 +360,34 @@ static int __register_pstore_blk(struct pstore_blk_info *info)
> }
>
> /* only allow driver matching the @blkdev */
> - if (!binfo.devt || (!best_effort &&
> - MAJOR(binfo.devt) != info->major)) {
> - pr_debug("invalid major %u (expect %u)\n",
> - info->major, MAJOR(binfo.devt));
> + if (!binfo.devt) {
> + pr_debug("no major\n");
> ret = -ENODEV;
> goto err_put_bdev;
> }
>
> /* psblk_bdev must be assigned before register to pstore/blk */
> psblk_bdev = bdev;
> - blkdev_panic_write = info->panic_write;
> -
> - /* Copy back block device details. */
> - info->devt = binfo.devt;
> - info->nr_sects = binfo.nr_sects;
> - info->start_sect = binfo.start_sect;
>
> memset(&dev, 0, sizeof(dev));
> - dev.total_size = info->nr_sects << SECTOR_SHIFT;
> - dev.flags = info->flags;
> + dev.total_size = binfo.nr_sects << SECTOR_SHIFT;
> dev.read = psblk_generic_blk_read;
> dev.write = psblk_generic_blk_write;
> - dev.erase = NULL;
> - dev.panic_write = info->panic_write ? psblk_blk_panic_write : NULL;
>
> ret = __register_pstore_device(&dev);
> if (ret)
> goto err_put_bdev;
>
> bdevname(bdev, bdev_name);
> - pr_info("attached %s%s\n", bdev_name,
> - info->panic_write ? "" : " (no dedicated panic_write!)");
> + pr_info("attached %s (no dedicated panic_write!)\n", bdev_name);
> return 0;
>
> err_put_bdev:
> psblk_bdev = NULL;
> - blkdev_panic_write = NULL;
> psblk_put_bdev(bdev, holder);
> return ret;
> }
>
> -/**
> - * register_pstore_blk() - register block device to pstore/blk
> - *
> - * @info: details on the desired block device interface
> - *
> - * Return:
> - * * 0 - OK
> - * * Others - something error.
> - */
> -int register_pstore_blk(struct pstore_blk_info *info)
> -{
> - int ret;
> -
> - mutex_lock(&pstore_blk_lock);
> - ret = __register_pstore_blk(info);
> - mutex_unlock(&pstore_blk_lock);
> -
> - return ret;
> -}
> -EXPORT_SYMBOL_GPL(register_pstore_blk);
> -
> static void __unregister_pstore_blk(unsigned int major)
> {
> struct pstore_device_info dev = { .read = psblk_generic_blk_read };
> @@ -449,24 +397,10 @@ static void __unregister_pstore_blk(unsigned int major)
> if (psblk_bdev && MAJOR(psblk_bdev->bd_dev) == major) {
> __unregister_pstore_device(&dev);
> psblk_put_bdev(psblk_bdev, holder);
> - blkdev_panic_write = NULL;
> psblk_bdev = NULL;
> }
> }
>
> -/**
> - * unregister_pstore_blk() - unregister block device from pstore/blk
> - *
> - * @major: the major device number of device
> - */
> -void unregister_pstore_blk(unsigned int major)
> -{
> - mutex_lock(&pstore_blk_lock);
> - __unregister_pstore_blk(major);
> - mutex_unlock(&pstore_blk_lock);
> -}
> -EXPORT_SYMBOL_GPL(unregister_pstore_blk);
> -
> /* get information of pstore/blk */
> int pstore_blk_get_config(struct pstore_blk_config *info)
> {
> @@ -483,12 +417,11 @@ EXPORT_SYMBOL_GPL(pstore_blk_get_config);
>
> static int __init pstore_blk_init(void)
> {
> - struct pstore_blk_info info = { };
> int ret = 0;
>
> mutex_lock(&pstore_blk_lock);
> if (!pstore_zone_info && best_effort && blkdev[0])
> - ret = __register_pstore_blk(&info);
> + ret = __register_pstore_blk();
> mutex_unlock(&pstore_blk_lock);
>
> return ret;
> diff --git a/include/linux/pstore_blk.h b/include/linux/pstore_blk.h
> index 61e914522b0193..99564f93d77488 100644
> --- a/include/linux/pstore_blk.h
> +++ b/include/linux/pstore_blk.h
> @@ -7,48 +7,6 @@
> #include <linux/pstore.h>
> #include <linux/pstore_zone.h>
>
> -/**
> - * typedef pstore_blk_panic_write_op - panic write operation to block device
> - *
> - * @buf: the data to write
> - * @start_sect: start sector to block device
> - * @sects: sectors count on buf
> - *
> - * Return: On success, zero should be returned. Others excluding -ENOMSG
> - * mean error. -ENOMSG means to try next zone.
> - *
> - * Panic write to block device must be aligned to SECTOR_SIZE.
> - */
> -typedef int (*pstore_blk_panic_write_op)(const char *buf, sector_t start_sect,
> - sector_t sects);
> -
> -/**
> - * struct pstore_blk_info - pstore/blk registration details
> - *
> - * @major: Which major device number to support with pstore/blk
> - * @flags: The supported PSTORE_FLAGS_* from linux/pstore.h.
> - * @panic_write:The write operation only used for the panic case.
> - * This can be NULL, but is recommended to avoid losing
> - * crash data if the kernel's IO path or work queues are
> - * broken during a panic.
> - * @devt: The dev_t that pstore/blk has attached to.
> - * @nr_sects: Number of sectors on @devt.
> - * @start_sect: Starting sector on @devt.
> - */
> -struct pstore_blk_info {
> - unsigned int major;
> - unsigned int flags;
> - pstore_blk_panic_write_op panic_write;
> -
> - /* Filled in by pstore/blk after registration. */
> - dev_t devt;
> - sector_t nr_sects;
> - sector_t start_sect;
> -};
> -
> -int register_pstore_blk(struct pstore_blk_info *info);
> -void unregister_pstore_blk(unsigned int major);
> -
> /**
> * struct pstore_device_info - back-end pstore/blk driver structure.
> *
> --
> 2.28.0
>
--
Kees Cook
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 4/9] pstore/blk: remove __unregister_pstore_blk
2020-10-16 13:20 simplify pstore-blk Christoph Hellwig
` (2 preceding siblings ...)
2020-10-16 13:20 ` [PATCH 3/9] pstore/blk: remove {un,}register_pstore_blk Christoph Hellwig
@ 2020-10-16 13:20 ` Christoph Hellwig
2020-12-01 19:20 ` Kees Cook
2020-10-16 13:20 ` [PATCH 5/9] pstore/blk: simplify the block device open / close path Christoph Hellwig
` (6 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Christoph Hellwig @ 2020-10-16 13:20 UTC (permalink / raw)
To: Kees Cook, Anton Vorontsov, Colin Cross, Tony Luck, WeiXiong Liao
Cc: linux-mtd, linux-kernel
Fold __unregister_pstore_blk into its only caller, and merge the
two identical calls to __unregister_pstore_device that exist in the
caller now.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/pstore/blk.c | 27 ++++++---------------------
1 file changed, 6 insertions(+), 21 deletions(-)
diff --git a/fs/pstore/blk.c b/fs/pstore/blk.c
index 7f8368e94b3604..3a2214ecf942ae 100644
--- a/fs/pstore/blk.c
+++ b/fs/pstore/blk.c
@@ -388,19 +388,6 @@ static int __register_pstore_blk(void)
return ret;
}
-static void __unregister_pstore_blk(unsigned int major)
-{
- struct pstore_device_info dev = { .read = psblk_generic_blk_read };
- void *holder = blkdev;
-
- lockdep_assert_held(&pstore_blk_lock);
- if (psblk_bdev && MAJOR(psblk_bdev->bd_dev) == major) {
- __unregister_pstore_device(&dev);
- psblk_put_bdev(psblk_bdev, holder);
- psblk_bdev = NULL;
- }
-}
-
/* get information of pstore/blk */
int pstore_blk_get_config(struct pstore_blk_config *info)
{
@@ -430,16 +417,14 @@ late_initcall(pstore_blk_init);
static void __exit pstore_blk_exit(void)
{
+ struct pstore_device_info dev = { };
+
mutex_lock(&pstore_blk_lock);
+ if (pstore_zone_info)
+ dev.read = pstore_zone_info->read;
+ __unregister_pstore_device(&dev);
if (psblk_bdev)
- __unregister_pstore_blk(MAJOR(psblk_bdev->bd_dev));
- else {
- struct pstore_device_info dev = { };
-
- if (pstore_zone_info)
- dev.read = pstore_zone_info->read;
- __unregister_pstore_device(&dev);
- }
+ psblk_put_bdev(psblk_bdev, blkdev);
mutex_unlock(&pstore_blk_lock);
}
module_exit(pstore_blk_exit);
--
2.28.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 4/9] pstore/blk: remove __unregister_pstore_blk
2020-10-16 13:20 ` [PATCH 4/9] pstore/blk: remove __unregister_pstore_blk Christoph Hellwig
@ 2020-12-01 19:20 ` Kees Cook
0 siblings, 0 replies; 28+ messages in thread
From: Kees Cook @ 2020-12-01 19:20 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Anton Vorontsov, Colin Cross, Tony Luck, WeiXiong Liao,
linux-mtd, linux-kernel
On Fri, Oct 16, 2020 at 03:20:42PM +0200, Christoph Hellwig wrote:
> Fold __unregister_pstore_blk into its only caller, and merge the
> two identical calls to __unregister_pstore_device that exist in the
> caller now.
Meh, I'm not a fan of this. I like it in a separate function. Pushing it
into the _exit() routine feels weird to me.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/pstore/blk.c | 27 ++++++---------------------
> 1 file changed, 6 insertions(+), 21 deletions(-)
>
> diff --git a/fs/pstore/blk.c b/fs/pstore/blk.c
> index 7f8368e94b3604..3a2214ecf942ae 100644
> --- a/fs/pstore/blk.c
> +++ b/fs/pstore/blk.c
> @@ -388,19 +388,6 @@ static int __register_pstore_blk(void)
> return ret;
> }
>
> -static void __unregister_pstore_blk(unsigned int major)
> -{
> - struct pstore_device_info dev = { .read = psblk_generic_blk_read };
> - void *holder = blkdev;
> -
> - lockdep_assert_held(&pstore_blk_lock);
> - if (psblk_bdev && MAJOR(psblk_bdev->bd_dev) == major) {
> - __unregister_pstore_device(&dev);
> - psblk_put_bdev(psblk_bdev, holder);
> - psblk_bdev = NULL;
> - }
> -}
> -
> /* get information of pstore/blk */
> int pstore_blk_get_config(struct pstore_blk_config *info)
> {
> @@ -430,16 +417,14 @@ late_initcall(pstore_blk_init);
>
> static void __exit pstore_blk_exit(void)
> {
> + struct pstore_device_info dev = { };
> +
> mutex_lock(&pstore_blk_lock);
> + if (pstore_zone_info)
> + dev.read = pstore_zone_info->read;
> + __unregister_pstore_device(&dev);
> if (psblk_bdev)
> - __unregister_pstore_blk(MAJOR(psblk_bdev->bd_dev));
> - else {
> - struct pstore_device_info dev = { };
> -
> - if (pstore_zone_info)
> - dev.read = pstore_zone_info->read;
> - __unregister_pstore_device(&dev);
> - }
> + psblk_put_bdev(psblk_bdev, blkdev);
> mutex_unlock(&pstore_blk_lock);
> }
> module_exit(pstore_blk_exit);
> --
> 2.28.0
>
--
Kees Cook
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 5/9] pstore/blk: simplify the block device open / close path
2020-10-16 13:20 simplify pstore-blk Christoph Hellwig
` (3 preceding siblings ...)
2020-10-16 13:20 ` [PATCH 4/9] pstore/blk: remove __unregister_pstore_blk Christoph Hellwig
@ 2020-10-16 13:20 ` Christoph Hellwig
2020-12-01 19:40 ` Kees Cook
2020-10-16 13:20 ` [PATCH 6/9] pstore/zone: split struct pstore_zone_info Christoph Hellwig
` (5 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Christoph Hellwig @ 2020-10-16 13:20 UTC (permalink / raw)
To: Kees Cook, Anton Vorontsov, Colin Cross, Tony Luck, WeiXiong Liao
Cc: linux-mtd, linux-kernel
Remove the pointless psblk_get_bdev and psblk_put_bdev helper,
and don't bother holding pstore_blk_lock over the block device
open / close interactions given that they only happen first thing
during module init and last thing during module exit. Also don't
bother with unregistering a zone info that did not come from the
actually block backed code, as users like mtd have to unregister
it themselves already.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/pstore/blk.c | 156 ++++++++++++------------------------------------
1 file changed, 38 insertions(+), 118 deletions(-)
diff --git a/fs/pstore/blk.c b/fs/pstore/blk.c
index 3a2214ecf942ae..a8aa56cba96d59 100644
--- a/fs/pstore/blk.c
+++ b/fs/pstore/blk.c
@@ -91,12 +91,6 @@ static DEFINE_MUTEX(pstore_blk_lock);
static struct block_device *psblk_bdev;
static struct pstore_zone_info *pstore_zone_info;
-struct bdev_info {
- dev_t devt;
- sector_t nr_sects;
- sector_t start_sect;
-};
-
#define check_size(name, alignsize) ({ \
long _##name_ = (name); \
_##name_ = _##name_ <= 0 ? 0 : (_##name_ * 1024); \
@@ -205,75 +199,6 @@ void unregister_pstore_device(struct pstore_device_info *dev)
}
EXPORT_SYMBOL_GPL(unregister_pstore_device);
-/**
- * psblk_get_bdev() - open block device
- *
- * @holder: Exclusive holder identifier
- * @info: Information about bdev to fill in
- *
- * Return: pointer to block device on success and others on error.
- *
- * On success, the returned block_device has reference count of one.
- */
-static struct block_device *psblk_get_bdev(void *holder,
- struct bdev_info *info)
-{
- struct block_device *bdev = ERR_PTR(-ENODEV);
- fmode_t mode = FMODE_READ | FMODE_WRITE;
- sector_t nr_sects;
-
- lockdep_assert_held(&pstore_blk_lock);
-
- if (pstore_zone_info)
- return ERR_PTR(-EBUSY);
-
- if (!blkdev[0])
- return ERR_PTR(-ENODEV);
-
- if (holder)
- mode |= FMODE_EXCL;
- bdev = blkdev_get_by_path(blkdev, mode, holder);
- if (IS_ERR(bdev)) {
- dev_t devt;
-
- devt = name_to_dev_t(blkdev);
- if (devt == 0)
- return ERR_PTR(-ENODEV);
- bdev = blkdev_get_by_dev(devt, mode, holder);
- if (IS_ERR(bdev))
- return bdev;
- }
-
- nr_sects = part_nr_sects_read(bdev->bd_part);
- if (!nr_sects) {
- pr_err("not enough space for '%s'\n", blkdev);
- blkdev_put(bdev, mode);
- return ERR_PTR(-ENOSPC);
- }
-
- if (info) {
- info->devt = bdev->bd_dev;
- info->nr_sects = nr_sects;
- info->start_sect = get_start_sect(bdev);
- }
-
- return bdev;
-}
-
-static void psblk_put_bdev(struct block_device *bdev, void *holder)
-{
- fmode_t mode = FMODE_READ | FMODE_WRITE;
-
- lockdep_assert_held(&pstore_blk_lock);
-
- if (!bdev)
- return;
-
- if (holder)
- mode |= FMODE_EXCL;
- blkdev_put(bdev, mode);
-}
-
static ssize_t psblk_generic_blk_read(char *buf, size_t bytes, loff_t pos)
{
struct block_device *bdev = psblk_bdev;
@@ -340,29 +265,39 @@ static ssize_t psblk_generic_blk_write(const char *buf, size_t bytes,
return ret;
}
-static int __register_pstore_blk(void)
+static int __init pstore_blk_init(void)
{
char bdev_name[BDEVNAME_SIZE];
struct block_device *bdev;
struct pstore_device_info dev;
- struct bdev_info binfo;
- void *holder = blkdev;
int ret = -ENODEV;
-
- lockdep_assert_held(&pstore_blk_lock);
+ fmode_t mode = FMODE_READ | FMODE_WRITE | FMODE_EXCL;
+ sector_t nr_sects;
+
+ if (!best_effort || !blkdev[0])
+ return 0;
/* hold bdev exclusively */
- memset(&binfo, 0, sizeof(binfo));
- bdev = psblk_get_bdev(holder, &binfo);
+ bdev = blkdev_get_by_path(blkdev, mode, blkdev);
if (IS_ERR(bdev)) {
- pr_err("failed to open '%s'!\n", blkdev);
- return PTR_ERR(bdev);
+ dev_t devt;
+
+ devt = name_to_dev_t(blkdev);
+ if (devt == 0) {
+ pr_err("failed to open '%s'!\n", blkdev);
+ return -ENODEV;
+ }
+ bdev = blkdev_get_by_dev(devt, mode, blkdev);
+ if (IS_ERR(bdev)) {
+ pr_err("failed to open '%s'!\n", blkdev);
+ return PTR_ERR(bdev);
+ }
}
- /* only allow driver matching the @blkdev */
- if (!binfo.devt) {
- pr_debug("no major\n");
- ret = -ENODEV;
+ nr_sects = part_nr_sects_read(bdev->bd_part);
+ if (!nr_sects) {
+ pr_err("not enough space for '%s'\n", blkdev);
+ ret = -ENOSPC;
goto err_put_bdev;
}
@@ -370,11 +305,11 @@ static int __register_pstore_blk(void)
psblk_bdev = bdev;
memset(&dev, 0, sizeof(dev));
- dev.total_size = binfo.nr_sects << SECTOR_SHIFT;
+ dev.total_size = nr_sects << SECTOR_SHIFT;
dev.read = psblk_generic_blk_read;
dev.write = psblk_generic_blk_write;
- ret = __register_pstore_device(&dev);
+ ret = register_pstore_device(&dev);
if (ret)
goto err_put_bdev;
@@ -383,10 +318,22 @@ static int __register_pstore_blk(void)
return 0;
err_put_bdev:
+ blkdev_put(bdev, FMODE_READ | FMODE_WRITE | FMODE_EXCL);
psblk_bdev = NULL;
- psblk_put_bdev(bdev, holder);
return ret;
}
+late_initcall(pstore_blk_init);
+
+static void __exit pstore_blk_exit(void)
+{
+ struct pstore_device_info dev = { .read = psblk_generic_blk_read };
+
+ if (!psblk_bdev)
+ return;
+ unregister_pstore_device(&dev);
+ blkdev_put(psblk_bdev, FMODE_READ | FMODE_WRITE | FMODE_EXCL);
+}
+module_exit(pstore_blk_exit);
/* get information of pstore/blk */
int pstore_blk_get_config(struct pstore_blk_config *info)
@@ -402,33 +349,6 @@ int pstore_blk_get_config(struct pstore_blk_config *info)
}
EXPORT_SYMBOL_GPL(pstore_blk_get_config);
-static int __init pstore_blk_init(void)
-{
- int ret = 0;
-
- mutex_lock(&pstore_blk_lock);
- if (!pstore_zone_info && best_effort && blkdev[0])
- ret = __register_pstore_blk();
- mutex_unlock(&pstore_blk_lock);
-
- return ret;
-}
-late_initcall(pstore_blk_init);
-
-static void __exit pstore_blk_exit(void)
-{
- struct pstore_device_info dev = { };
-
- mutex_lock(&pstore_blk_lock);
- if (pstore_zone_info)
- dev.read = pstore_zone_info->read;
- __unregister_pstore_device(&dev);
- if (psblk_bdev)
- psblk_put_bdev(psblk_bdev, blkdev);
- mutex_unlock(&pstore_blk_lock);
-}
-module_exit(pstore_blk_exit);
-
MODULE_LICENSE("GPL");
MODULE_AUTHOR("WeiXiong Liao <liaoweixiong@allwinnertech.com>");
MODULE_AUTHOR("Kees Cook <keescook@chromium.org>");
--
2.28.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 5/9] pstore/blk: simplify the block device open / close path
2020-10-16 13:20 ` [PATCH 5/9] pstore/blk: simplify the block device open / close path Christoph Hellwig
@ 2020-12-01 19:40 ` Kees Cook
0 siblings, 0 replies; 28+ messages in thread
From: Kees Cook @ 2020-12-01 19:40 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Anton Vorontsov, Colin Cross, Tony Luck, WeiXiong Liao,
linux-mtd, linux-kernel
On Fri, Oct 16, 2020 at 03:20:43PM +0200, Christoph Hellwig wrote:
> Remove the pointless psblk_get_bdev and psblk_put_bdev helper,
> and don't bother holding pstore_blk_lock over the block device
> open / close interactions given that they only happen first thing
> during module init and last thing during module exit. Also don't
> bother with unregistering a zone info that did not come from the
> actually block backed code, as users like mtd have to unregister
> it themselves already.
I don't like this because it's changing too many things at once. Of the
stuff I don't want is:
- removal of checking for already-active zoneinfo (there can be only one)
- collapsing register_blk into _init (it seems clearer to me to keep
them separate)
-Kees
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/pstore/blk.c | 156 ++++++++++++------------------------------------
> 1 file changed, 38 insertions(+), 118 deletions(-)
>
> diff --git a/fs/pstore/blk.c b/fs/pstore/blk.c
> index 3a2214ecf942ae..a8aa56cba96d59 100644
> --- a/fs/pstore/blk.c
> +++ b/fs/pstore/blk.c
> @@ -91,12 +91,6 @@ static DEFINE_MUTEX(pstore_blk_lock);
> static struct block_device *psblk_bdev;
> static struct pstore_zone_info *pstore_zone_info;
>
> -struct bdev_info {
> - dev_t devt;
> - sector_t nr_sects;
> - sector_t start_sect;
> -};
> -
> #define check_size(name, alignsize) ({ \
> long _##name_ = (name); \
> _##name_ = _##name_ <= 0 ? 0 : (_##name_ * 1024); \
> @@ -205,75 +199,6 @@ void unregister_pstore_device(struct pstore_device_info *dev)
> }
> EXPORT_SYMBOL_GPL(unregister_pstore_device);
>
> -/**
> - * psblk_get_bdev() - open block device
> - *
> - * @holder: Exclusive holder identifier
> - * @info: Information about bdev to fill in
> - *
> - * Return: pointer to block device on success and others on error.
> - *
> - * On success, the returned block_device has reference count of one.
> - */
> -static struct block_device *psblk_get_bdev(void *holder,
> - struct bdev_info *info)
> -{
> - struct block_device *bdev = ERR_PTR(-ENODEV);
> - fmode_t mode = FMODE_READ | FMODE_WRITE;
> - sector_t nr_sects;
> -
> - lockdep_assert_held(&pstore_blk_lock);
> -
> - if (pstore_zone_info)
> - return ERR_PTR(-EBUSY);
> -
> - if (!blkdev[0])
> - return ERR_PTR(-ENODEV);
> -
> - if (holder)
> - mode |= FMODE_EXCL;
> - bdev = blkdev_get_by_path(blkdev, mode, holder);
> - if (IS_ERR(bdev)) {
> - dev_t devt;
> -
> - devt = name_to_dev_t(blkdev);
> - if (devt == 0)
> - return ERR_PTR(-ENODEV);
> - bdev = blkdev_get_by_dev(devt, mode, holder);
> - if (IS_ERR(bdev))
> - return bdev;
> - }
> -
> - nr_sects = part_nr_sects_read(bdev->bd_part);
> - if (!nr_sects) {
> - pr_err("not enough space for '%s'\n", blkdev);
> - blkdev_put(bdev, mode);
> - return ERR_PTR(-ENOSPC);
> - }
> -
> - if (info) {
> - info->devt = bdev->bd_dev;
> - info->nr_sects = nr_sects;
> - info->start_sect = get_start_sect(bdev);
> - }
> -
> - return bdev;
> -}
> -
> -static void psblk_put_bdev(struct block_device *bdev, void *holder)
> -{
> - fmode_t mode = FMODE_READ | FMODE_WRITE;
> -
> - lockdep_assert_held(&pstore_blk_lock);
> -
> - if (!bdev)
> - return;
> -
> - if (holder)
> - mode |= FMODE_EXCL;
> - blkdev_put(bdev, mode);
> -}
> -
> static ssize_t psblk_generic_blk_read(char *buf, size_t bytes, loff_t pos)
> {
> struct block_device *bdev = psblk_bdev;
> @@ -340,29 +265,39 @@ static ssize_t psblk_generic_blk_write(const char *buf, size_t bytes,
> return ret;
> }
>
> -static int __register_pstore_blk(void)
> +static int __init pstore_blk_init(void)
> {
> char bdev_name[BDEVNAME_SIZE];
> struct block_device *bdev;
> struct pstore_device_info dev;
> - struct bdev_info binfo;
> - void *holder = blkdev;
> int ret = -ENODEV;
> -
> - lockdep_assert_held(&pstore_blk_lock);
> + fmode_t mode = FMODE_READ | FMODE_WRITE | FMODE_EXCL;
> + sector_t nr_sects;
> +
> + if (!best_effort || !blkdev[0])
> + return 0;
>
> /* hold bdev exclusively */
> - memset(&binfo, 0, sizeof(binfo));
> - bdev = psblk_get_bdev(holder, &binfo);
> + bdev = blkdev_get_by_path(blkdev, mode, blkdev);
> if (IS_ERR(bdev)) {
> - pr_err("failed to open '%s'!\n", blkdev);
> - return PTR_ERR(bdev);
> + dev_t devt;
> +
> + devt = name_to_dev_t(blkdev);
> + if (devt == 0) {
> + pr_err("failed to open '%s'!\n", blkdev);
> + return -ENODEV;
> + }
> + bdev = blkdev_get_by_dev(devt, mode, blkdev);
> + if (IS_ERR(bdev)) {
> + pr_err("failed to open '%s'!\n", blkdev);
> + return PTR_ERR(bdev);
> + }
> }
>
> - /* only allow driver matching the @blkdev */
> - if (!binfo.devt) {
> - pr_debug("no major\n");
> - ret = -ENODEV;
> + nr_sects = part_nr_sects_read(bdev->bd_part);
> + if (!nr_sects) {
> + pr_err("not enough space for '%s'\n", blkdev);
> + ret = -ENOSPC;
> goto err_put_bdev;
> }
>
> @@ -370,11 +305,11 @@ static int __register_pstore_blk(void)
> psblk_bdev = bdev;
>
> memset(&dev, 0, sizeof(dev));
> - dev.total_size = binfo.nr_sects << SECTOR_SHIFT;
> + dev.total_size = nr_sects << SECTOR_SHIFT;
> dev.read = psblk_generic_blk_read;
> dev.write = psblk_generic_blk_write;
>
> - ret = __register_pstore_device(&dev);
> + ret = register_pstore_device(&dev);
> if (ret)
> goto err_put_bdev;
>
> @@ -383,10 +318,22 @@ static int __register_pstore_blk(void)
> return 0;
>
> err_put_bdev:
> + blkdev_put(bdev, FMODE_READ | FMODE_WRITE | FMODE_EXCL);
> psblk_bdev = NULL;
> - psblk_put_bdev(bdev, holder);
> return ret;
> }
> +late_initcall(pstore_blk_init);
> +
> +static void __exit pstore_blk_exit(void)
> +{
> + struct pstore_device_info dev = { .read = psblk_generic_blk_read };
> +
> + if (!psblk_bdev)
> + return;
> + unregister_pstore_device(&dev);
> + blkdev_put(psblk_bdev, FMODE_READ | FMODE_WRITE | FMODE_EXCL);
> +}
> +module_exit(pstore_blk_exit);
>
> /* get information of pstore/blk */
> int pstore_blk_get_config(struct pstore_blk_config *info)
> @@ -402,33 +349,6 @@ int pstore_blk_get_config(struct pstore_blk_config *info)
> }
> EXPORT_SYMBOL_GPL(pstore_blk_get_config);
>
> -static int __init pstore_blk_init(void)
> -{
> - int ret = 0;
> -
> - mutex_lock(&pstore_blk_lock);
> - if (!pstore_zone_info && best_effort && blkdev[0])
> - ret = __register_pstore_blk();
> - mutex_unlock(&pstore_blk_lock);
> -
> - return ret;
> -}
> -late_initcall(pstore_blk_init);
> -
> -static void __exit pstore_blk_exit(void)
> -{
> - struct pstore_device_info dev = { };
> -
> - mutex_lock(&pstore_blk_lock);
> - if (pstore_zone_info)
> - dev.read = pstore_zone_info->read;
> - __unregister_pstore_device(&dev);
> - if (psblk_bdev)
> - psblk_put_bdev(psblk_bdev, blkdev);
> - mutex_unlock(&pstore_blk_lock);
> -}
> -module_exit(pstore_blk_exit);
> -
> MODULE_LICENSE("GPL");
> MODULE_AUTHOR("WeiXiong Liao <liaoweixiong@allwinnertech.com>");
> MODULE_AUTHOR("Kees Cook <keescook@chromium.org>");
> --
> 2.28.0
>
--
Kees Cook
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 6/9] pstore/zone: split struct pstore_zone_info
2020-10-16 13:20 simplify pstore-blk Christoph Hellwig
` (4 preceding siblings ...)
2020-10-16 13:20 ` [PATCH 5/9] pstore/blk: simplify the block device open / close path Christoph Hellwig
@ 2020-10-16 13:20 ` Christoph Hellwig
2020-12-01 19:42 ` Kees Cook
2020-10-16 13:20 ` [PATCH 7/9] pstore/blk: remove struct pstore_device_info Christoph Hellwig
` (4 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Christoph Hellwig @ 2020-10-16 13:20 UTC (permalink / raw)
To: Kees Cook, Anton Vorontsov, Colin Cross, Tony Luck, WeiXiong Liao
Cc: linux-mtd, linux-kernel
Split out a new pstore_zone_ops structure for static function pointers
plus the name. Also remove the unused owner field entirely.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/mtd/mtdpstore.c | 13 +++++++----
fs/pstore/blk.c | 23 ++++++++++---------
fs/pstore/zone.c | 46 +++++++++++++++++++------------------
include/linux/pstore_blk.h | 23 ++-----------------
include/linux/pstore_zone.h | 41 ++++++++++++++++++---------------
5 files changed, 69 insertions(+), 77 deletions(-)
diff --git a/drivers/mtd/mtdpstore.c b/drivers/mtd/mtdpstore.c
index a3ae8778f6a9b9..232ba5c39c2a55 100644
--- a/drivers/mtd/mtdpstore.c
+++ b/drivers/mtd/mtdpstore.c
@@ -378,6 +378,14 @@ static ssize_t mtdpstore_panic_write(const char *buf, size_t size, loff_t off)
return retlen;
}
+static const struct pstore_zone_ops mtdpstore_ops = {
+ .name = KBUILD_MODNAME,
+ .read = mtdpstore_read,
+ .write = mtdpstore_write,
+ .erase = mtdpstore_erase,
+ .panic_write = mtdpstore_panic_write,
+};
+
static void mtdpstore_notify_add(struct mtd_info *mtd)
{
int ret;
@@ -426,10 +434,7 @@ static void mtdpstore_notify_add(struct mtd_info *mtd)
cxt->dev.total_size = mtd->size;
/* just support dmesg right now */
cxt->dev.flags = PSTORE_FLAGS_DMESG;
- cxt->dev.read = mtdpstore_read;
- cxt->dev.write = mtdpstore_write;
- cxt->dev.erase = mtdpstore_erase;
- cxt->dev.panic_write = mtdpstore_panic_write;
+ cxt->dev.ops = &mtdpstore_ops;
ret = register_pstore_device(&cxt->dev);
if (ret) {
diff --git a/fs/pstore/blk.c b/fs/pstore/blk.c
index a8aa56cba96d59..f7c7f325e42c71 100644
--- a/fs/pstore/blk.c
+++ b/fs/pstore/blk.c
@@ -108,7 +108,8 @@ static int __register_pstore_device(struct pstore_device_info *dev)
lockdep_assert_held(&pstore_blk_lock);
- if (!dev || !dev->total_size || !dev->read || !dev->write)
+ if (!dev || !dev->total_size || !dev->ops ||
+ !dev->ops->read || !dev->ops->write)
return -EINVAL;
/* someone already registered before */
@@ -141,12 +142,7 @@ static int __register_pstore_device(struct pstore_device_info *dev)
pstore_zone_info->total_size = dev->total_size;
pstore_zone_info->max_reason = max_reason;
- pstore_zone_info->read = dev->read;
- pstore_zone_info->write = dev->write;
- pstore_zone_info->erase = dev->erase;
- pstore_zone_info->panic_write = dev->panic_write;
- pstore_zone_info->name = KBUILD_MODNAME;
- pstore_zone_info->owner = THIS_MODULE;
+ pstore_zone_info->ops = dev->ops;
ret = register_pstore_zone(pstore_zone_info);
if (ret) {
@@ -179,7 +175,7 @@ EXPORT_SYMBOL_GPL(register_pstore_device);
static void __unregister_pstore_device(struct pstore_device_info *dev)
{
lockdep_assert_held(&pstore_blk_lock);
- if (pstore_zone_info && pstore_zone_info->read == dev->read) {
+ if (pstore_zone_info && pstore_zone_info->ops == dev->ops) {
unregister_pstore_zone(pstore_zone_info);
kfree(pstore_zone_info);
pstore_zone_info = NULL;
@@ -265,6 +261,12 @@ static ssize_t psblk_generic_blk_write(const char *buf, size_t bytes,
return ret;
}
+static const struct pstore_zone_ops pstore_blk_zone_ops = {
+ .name = KBUILD_MODNAME,
+ .read = psblk_generic_blk_read,
+ .write = psblk_generic_blk_write,
+};
+
static int __init pstore_blk_init(void)
{
char bdev_name[BDEVNAME_SIZE];
@@ -305,9 +307,8 @@ static int __init pstore_blk_init(void)
psblk_bdev = bdev;
memset(&dev, 0, sizeof(dev));
+ dev.ops = &pstore_blk_zone_ops;
dev.total_size = nr_sects << SECTOR_SHIFT;
- dev.read = psblk_generic_blk_read;
- dev.write = psblk_generic_blk_write;
ret = register_pstore_device(&dev);
if (ret)
@@ -326,7 +327,7 @@ late_initcall(pstore_blk_init);
static void __exit pstore_blk_exit(void)
{
- struct pstore_device_info dev = { .read = psblk_generic_blk_read };
+ struct pstore_device_info dev = { .ops = &pstore_blk_zone_ops };
if (!psblk_bdev)
return;
diff --git a/fs/pstore/zone.c b/fs/pstore/zone.c
index 5266ccbec007f3..111d26bb2a8f56 100644
--- a/fs/pstore/zone.c
+++ b/fs/pstore/zone.c
@@ -218,7 +218,7 @@ static int psz_zone_write(struct pstore_zone *zone,
if (!is_on_panic() && !atomic_read(&pstore_zone_cxt.recovered))
goto dirty;
- writeop = is_on_panic() ? info->panic_write : info->write;
+ writeop = is_on_panic() ? info->ops->panic_write : info->ops->write;
if (!writeop)
goto dirty;
@@ -337,7 +337,7 @@ static int psz_kmsg_recover_data(struct psz_context *cxt)
unsigned long i;
ssize_t rcnt;
- if (!info->read)
+ if (!info->ops->read)
return -EINVAL;
for (i = 0; i < cxt->kmsg_max_cnt; i++) {
@@ -360,8 +360,8 @@ static int psz_kmsg_recover_data(struct psz_context *cxt)
if (!zone->should_recover)
continue;
buf = zone->buffer;
- rcnt = info->read((char *)buf, zone->buffer_size + sizeof(*buf),
- zone->off);
+ rcnt = info->ops->read((char *)buf, zone->buffer_size +
+ sizeof(*buf), zone->off);
if (rcnt != zone->buffer_size + sizeof(*buf))
return (int)rcnt < 0 ? (int)rcnt : -EIO;
}
@@ -383,7 +383,7 @@ static int psz_kmsg_recover_meta(struct psz_context *cxt)
*/
char buffer_header[sizeof(*buf) + sizeof(*hdr)] = {0};
- if (!info->read)
+ if (!info->ops->read)
return -EINVAL;
len = sizeof(*buf) + sizeof(*hdr);
@@ -393,13 +393,14 @@ static int psz_kmsg_recover_meta(struct psz_context *cxt)
if (unlikely(!zone))
return -EINVAL;
- rcnt = info->read((char *)buf, len, zone->off);
+ rcnt = info->ops->read((char *)buf, len, zone->off);
if (rcnt == -ENOMSG) {
pr_debug("%s with id %lu may be broken, skip\n",
zone->name, i);
continue;
} else if (rcnt != len) {
- pr_err("read %s with id %lu failed\n", zone->name, i);
+ pr_err("read %s with id %lu failed\n", zone->name,
+ i);
return (int)rcnt < 0 ? (int)rcnt : -EIO;
}
@@ -495,11 +496,11 @@ static int psz_recover_zone(struct psz_context *cxt, struct pstore_zone *zone)
return 0;
}
- if (unlikely(!info->read))
+ if (unlikely(!info->ops->read))
return -EINVAL;
len = sizeof(struct psz_buffer);
- rcnt = info->read((char *)&tmpbuf, len, zone->off);
+ rcnt = info->ops->read((char *)&tmpbuf, len, zone->off);
if (rcnt != len) {
pr_debug("read zone %s failed\n", zone->name);
return (int)rcnt < 0 ? (int)rcnt : -EIO;
@@ -541,7 +542,7 @@ static int psz_recover_zone(struct psz_context *cxt, struct pstore_zone *zone)
off = zone->off + sizeof(*oldbuf);
/* get part of data */
- rcnt = info->read(buf, len - start, off + start);
+ rcnt = info->ops->read(buf, len - start, off + start);
if (rcnt != len - start) {
pr_err("read zone %s failed\n", zone->name);
ret = (int)rcnt < 0 ? (int)rcnt : -EIO;
@@ -549,7 +550,7 @@ static int psz_recover_zone(struct psz_context *cxt, struct pstore_zone *zone)
}
/* get the rest of data */
- rcnt = info->read(buf + len - start, start, off);
+ rcnt = info->ops->read(buf + len - start, start, off);
if (rcnt != start) {
pr_err("read zone %s failed\n", zone->name);
ret = (int)rcnt < 0 ? (int)rcnt : -EIO;
@@ -671,8 +672,8 @@ static inline int psz_kmsg_erase(struct psz_context *cxt,
size = buffer_datalen(zone) + sizeof(*zone->buffer);
atomic_set(&zone->buffer->datalen, 0);
- if (cxt->pstore_zone_info->erase)
- return cxt->pstore_zone_info->erase(size, zone->off);
+ if (cxt->pstore_zone_info->ops->erase)
+ return cxt->pstore_zone_info->ops->erase(size, zone->off);
else
return psz_zone_write(zone, FLUSH_META, NULL, 0, 0);
}
@@ -1185,8 +1186,9 @@ static struct pstore_zone *psz_init_zone(enum pstore_type_id type,
*off += size;
- pr_debug("pszone %s: off 0x%llx, %zu header, %zu data\n", zone->name,
- zone->off, sizeof(*zone->buffer), zone->buffer_size);
+ pr_debug("pszone %s: off 0x%llx, %zu header, %zu data\n",
+ zone->name, zone->off, sizeof(*zone->buffer),
+ zone->buffer_size);
return zone;
}
@@ -1310,7 +1312,7 @@ int register_pstore_zone(struct pstore_zone_info *info)
return -EINVAL;
}
- if (!info->name || !info->name[0])
+ if (!info->ops->name || !info->ops->name[0])
return -EINVAL;
#define check_size(name, size) { \
@@ -1338,7 +1340,7 @@ int register_pstore_zone(struct pstore_zone_info *info)
* if no @read, pstore may mount failed.
* if no @write, pstore do not support to remove record file.
*/
- if (!info->read || !info->write) {
+ if (!info->ops || !info->ops->read || !info->ops->write) {
pr_err("no valid general read/write interface\n");
return -EINVAL;
}
@@ -1346,13 +1348,13 @@ int register_pstore_zone(struct pstore_zone_info *info)
mutex_lock(&cxt->pstore_zone_info_lock);
if (cxt->pstore_zone_info) {
pr_warn("'%s' already loaded: ignoring '%s'\n",
- cxt->pstore_zone_info->name, info->name);
+ cxt->pstore_zone_info->ops->name, info->ops->name);
mutex_unlock(&cxt->pstore_zone_info_lock);
return -EBUSY;
}
cxt->pstore_zone_info = info;
- pr_debug("register %s with properties:\n", info->name);
+ pr_debug("register %s with properties:\n", info->ops->name);
pr_debug("\ttotal size : %ld Bytes\n", info->total_size);
pr_debug("\tkmsg size : %ld Bytes\n", info->kmsg_size);
pr_debug("\tpmsg size : %ld Bytes\n", info->pmsg_size);
@@ -1376,14 +1378,14 @@ int register_pstore_zone(struct pstore_zone_info *info)
}
cxt->pstore.data = cxt;
- pr_info("registered %s as backend for", info->name);
+ pr_info("registered %s as backend for", info->ops->name);
cxt->pstore.max_reason = info->max_reason;
- cxt->pstore.name = info->name;
+ cxt->pstore.name = info->ops->name;
if (info->kmsg_size) {
cxt->pstore.flags |= PSTORE_FLAGS_DMESG;
pr_cont(" kmsg(%s",
kmsg_dump_reason_str(cxt->pstore.max_reason));
- if (cxt->pstore_zone_info->panic_write)
+ if (cxt->pstore_zone_info->ops->panic_write)
pr_cont(",panic_write");
pr_cont(")");
}
diff --git a/include/linux/pstore_blk.h b/include/linux/pstore_blk.h
index 99564f93d77488..095a44ce5e122c 100644
--- a/include/linux/pstore_blk.h
+++ b/include/linux/pstore_blk.h
@@ -15,31 +15,12 @@
* @flags: Refer to macro starting with PSTORE_FLAGS defined in
* linux/pstore.h. It means what front-ends this device support.
* Zero means all backends for compatible.
- * @read: The general read operation. Both of the function parameters
- * @size and @offset are relative value to bock device (not the
- * whole disk).
- * On success, the number of bytes should be returned, others
- * means error.
- * @write: The same as @read, but the following error number:
- * -EBUSY means try to write again later.
- * -ENOMSG means to try next zone.
- * @erase: The general erase operation for device with special removing
- * job. Both of the function parameters @size and @offset are
- * relative value to storage.
- * Return 0 on success and others on failure.
- * @panic_write:The write operation only used for panic case. It's optional
- * if you do not care panic log. The parameters are relative
- * value to storage.
- * On success, the number of bytes should be returned, others
- * excluding -ENOMSG mean error. -ENOMSG means to try next zone.
+ * @ops: operations to access the device.
*/
struct pstore_device_info {
unsigned long total_size;
unsigned int flags;
- pstore_zone_read_op read;
- pstore_zone_write_op write;
- pstore_zone_erase_op erase;
- pstore_zone_write_op panic_write;
+ const struct pstore_zone_ops *ops;
};
int register_pstore_device(struct pstore_device_info *dev);
diff --git a/include/linux/pstore_zone.h b/include/linux/pstore_zone.h
index 1e35eaa33e5e0d..7bff124c96d8b1 100644
--- a/include/linux/pstore_zone.h
+++ b/include/linux/pstore_zone.h
@@ -5,22 +5,10 @@
#include <linux/types.h>
-typedef ssize_t (*pstore_zone_read_op)(char *, size_t, loff_t);
-typedef ssize_t (*pstore_zone_write_op)(const char *, size_t, loff_t);
-typedef ssize_t (*pstore_zone_erase_op)(size_t, loff_t);
/**
- * struct pstore_zone_info - pstore/zone back-end driver structure
+ * struct pstore_zone_ops - pstore/zone ops structure
*
- * @owner: Module which is responsible for this back-end driver.
* @name: Name of the back-end driver.
- * @total_size: The total size in bytes pstore/zone can use. It must be greater
- * than 4096 and be multiple of 4096.
- * @kmsg_size: The size of oops/panic zone. Zero means disabled, otherwise,
- * it must be multiple of SECTOR_SIZE(512 Bytes).
- * @max_reason: Maximum kmsg dump reason to store.
- * @pmsg_size: The size of pmsg zone which is the same as @kmsg_size.
- * @console_size:The size of console zone which is the same as @kmsg_size.
- * @ftrace_size:The size of ftrace zone which is the same as @kmsg_size.
* @read: The general read operation. Both of the function parameters
* @size and @offset are relative value to storage.
* On success, the number of bytes should be returned, others
@@ -38,20 +26,35 @@ typedef ssize_t (*pstore_zone_erase_op)(size_t, loff_t);
* On success, the number of bytes should be returned, others
* excluding -ENOMSG mean error. -ENOMSG means to try next zone.
*/
-struct pstore_zone_info {
- struct module *owner;
+struct pstore_zone_ops {
const char *name;
+ ssize_t (*read)(char *buf, size_t count, loff_t pos);
+ ssize_t (*write)(const char *buf, size_t bytes, loff_t pos);
+ ssize_t (*erase)(size_t byes, loff_t pos);
+ ssize_t (*panic_write)(const char *buf, size_t bytes, loff_t pos);
+};
+/**
+ * struct pstore_zone_info - pstore/zone back-end driver structure
+ *
+ * @ops: Operations to access the zone.
+ * @total_size: The total size in bytes pstore/zone can use. It must be greater
+ * than 4096 and be multiple of 4096.
+ * @kmsg_size: The size of oops/panic zone. Zero means disabled, otherwise,
+ * it must be multiple of SECTOR_SIZE(512 Bytes).
+ * @max_reason: Maximum kmsg dump reason to store.
+ * @pmsg_size: The size of pmsg zone which is the same as @kmsg_size.
+ * @console_size:The size of console zone which is the same as @kmsg_size.
+ * @ftrace_size:The size of ftrace zone which is the same as @kmsg_size.
+ */
+struct pstore_zone_info {
+ const struct pstore_zone_ops *ops;
unsigned long total_size;
unsigned long kmsg_size;
int max_reason;
unsigned long pmsg_size;
unsigned long console_size;
unsigned long ftrace_size;
- pstore_zone_read_op read;
- pstore_zone_write_op write;
- pstore_zone_erase_op erase;
- pstore_zone_write_op panic_write;
};
extern int register_pstore_zone(struct pstore_zone_info *info);
--
2.28.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 6/9] pstore/zone: split struct pstore_zone_info
2020-10-16 13:20 ` [PATCH 6/9] pstore/zone: split struct pstore_zone_info Christoph Hellwig
@ 2020-12-01 19:42 ` Kees Cook
0 siblings, 0 replies; 28+ messages in thread
From: Kees Cook @ 2020-12-01 19:42 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Anton Vorontsov, Colin Cross, Tony Luck, WeiXiong Liao,
linux-mtd, linux-kernel
On Fri, Oct 16, 2020 at 03:20:44PM +0200, Christoph Hellwig wrote:
> Split out a new pstore_zone_ops structure for static function pointers
> plus the name. Also remove the unused owner field entirely.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
On the face of it, this seems fine, but I don't think it's really useful
to split the structure up. There's a lot of churn here.
-Kees
> ---
> drivers/mtd/mtdpstore.c | 13 +++++++----
> fs/pstore/blk.c | 23 ++++++++++---------
> fs/pstore/zone.c | 46 +++++++++++++++++++------------------
> include/linux/pstore_blk.h | 23 ++-----------------
> include/linux/pstore_zone.h | 41 ++++++++++++++++++---------------
> 5 files changed, 69 insertions(+), 77 deletions(-)
>
> diff --git a/drivers/mtd/mtdpstore.c b/drivers/mtd/mtdpstore.c
> index a3ae8778f6a9b9..232ba5c39c2a55 100644
> --- a/drivers/mtd/mtdpstore.c
> +++ b/drivers/mtd/mtdpstore.c
> @@ -378,6 +378,14 @@ static ssize_t mtdpstore_panic_write(const char *buf, size_t size, loff_t off)
> return retlen;
> }
>
> +static const struct pstore_zone_ops mtdpstore_ops = {
> + .name = KBUILD_MODNAME,
> + .read = mtdpstore_read,
> + .write = mtdpstore_write,
> + .erase = mtdpstore_erase,
> + .panic_write = mtdpstore_panic_write,
> +};
> +
> static void mtdpstore_notify_add(struct mtd_info *mtd)
> {
> int ret;
> @@ -426,10 +434,7 @@ static void mtdpstore_notify_add(struct mtd_info *mtd)
> cxt->dev.total_size = mtd->size;
> /* just support dmesg right now */
> cxt->dev.flags = PSTORE_FLAGS_DMESG;
> - cxt->dev.read = mtdpstore_read;
> - cxt->dev.write = mtdpstore_write;
> - cxt->dev.erase = mtdpstore_erase;
> - cxt->dev.panic_write = mtdpstore_panic_write;
> + cxt->dev.ops = &mtdpstore_ops;
>
> ret = register_pstore_device(&cxt->dev);
> if (ret) {
> diff --git a/fs/pstore/blk.c b/fs/pstore/blk.c
> index a8aa56cba96d59..f7c7f325e42c71 100644
> --- a/fs/pstore/blk.c
> +++ b/fs/pstore/blk.c
> @@ -108,7 +108,8 @@ static int __register_pstore_device(struct pstore_device_info *dev)
>
> lockdep_assert_held(&pstore_blk_lock);
>
> - if (!dev || !dev->total_size || !dev->read || !dev->write)
> + if (!dev || !dev->total_size || !dev->ops ||
> + !dev->ops->read || !dev->ops->write)
> return -EINVAL;
>
> /* someone already registered before */
> @@ -141,12 +142,7 @@ static int __register_pstore_device(struct pstore_device_info *dev)
>
> pstore_zone_info->total_size = dev->total_size;
> pstore_zone_info->max_reason = max_reason;
> - pstore_zone_info->read = dev->read;
> - pstore_zone_info->write = dev->write;
> - pstore_zone_info->erase = dev->erase;
> - pstore_zone_info->panic_write = dev->panic_write;
> - pstore_zone_info->name = KBUILD_MODNAME;
> - pstore_zone_info->owner = THIS_MODULE;
> + pstore_zone_info->ops = dev->ops;
>
> ret = register_pstore_zone(pstore_zone_info);
> if (ret) {
> @@ -179,7 +175,7 @@ EXPORT_SYMBOL_GPL(register_pstore_device);
> static void __unregister_pstore_device(struct pstore_device_info *dev)
> {
> lockdep_assert_held(&pstore_blk_lock);
> - if (pstore_zone_info && pstore_zone_info->read == dev->read) {
> + if (pstore_zone_info && pstore_zone_info->ops == dev->ops) {
> unregister_pstore_zone(pstore_zone_info);
> kfree(pstore_zone_info);
> pstore_zone_info = NULL;
> @@ -265,6 +261,12 @@ static ssize_t psblk_generic_blk_write(const char *buf, size_t bytes,
> return ret;
> }
>
> +static const struct pstore_zone_ops pstore_blk_zone_ops = {
> + .name = KBUILD_MODNAME,
> + .read = psblk_generic_blk_read,
> + .write = psblk_generic_blk_write,
> +};
> +
> static int __init pstore_blk_init(void)
> {
> char bdev_name[BDEVNAME_SIZE];
> @@ -305,9 +307,8 @@ static int __init pstore_blk_init(void)
> psblk_bdev = bdev;
>
> memset(&dev, 0, sizeof(dev));
> + dev.ops = &pstore_blk_zone_ops;
> dev.total_size = nr_sects << SECTOR_SHIFT;
> - dev.read = psblk_generic_blk_read;
> - dev.write = psblk_generic_blk_write;
>
> ret = register_pstore_device(&dev);
> if (ret)
> @@ -326,7 +327,7 @@ late_initcall(pstore_blk_init);
>
> static void __exit pstore_blk_exit(void)
> {
> - struct pstore_device_info dev = { .read = psblk_generic_blk_read };
> + struct pstore_device_info dev = { .ops = &pstore_blk_zone_ops };
>
> if (!psblk_bdev)
> return;
> diff --git a/fs/pstore/zone.c b/fs/pstore/zone.c
> index 5266ccbec007f3..111d26bb2a8f56 100644
> --- a/fs/pstore/zone.c
> +++ b/fs/pstore/zone.c
> @@ -218,7 +218,7 @@ static int psz_zone_write(struct pstore_zone *zone,
> if (!is_on_panic() && !atomic_read(&pstore_zone_cxt.recovered))
> goto dirty;
>
> - writeop = is_on_panic() ? info->panic_write : info->write;
> + writeop = is_on_panic() ? info->ops->panic_write : info->ops->write;
> if (!writeop)
> goto dirty;
>
> @@ -337,7 +337,7 @@ static int psz_kmsg_recover_data(struct psz_context *cxt)
> unsigned long i;
> ssize_t rcnt;
>
> - if (!info->read)
> + if (!info->ops->read)
> return -EINVAL;
>
> for (i = 0; i < cxt->kmsg_max_cnt; i++) {
> @@ -360,8 +360,8 @@ static int psz_kmsg_recover_data(struct psz_context *cxt)
> if (!zone->should_recover)
> continue;
> buf = zone->buffer;
> - rcnt = info->read((char *)buf, zone->buffer_size + sizeof(*buf),
> - zone->off);
> + rcnt = info->ops->read((char *)buf, zone->buffer_size +
> + sizeof(*buf), zone->off);
> if (rcnt != zone->buffer_size + sizeof(*buf))
> return (int)rcnt < 0 ? (int)rcnt : -EIO;
> }
> @@ -383,7 +383,7 @@ static int psz_kmsg_recover_meta(struct psz_context *cxt)
> */
> char buffer_header[sizeof(*buf) + sizeof(*hdr)] = {0};
>
> - if (!info->read)
> + if (!info->ops->read)
> return -EINVAL;
>
> len = sizeof(*buf) + sizeof(*hdr);
> @@ -393,13 +393,14 @@ static int psz_kmsg_recover_meta(struct psz_context *cxt)
> if (unlikely(!zone))
> return -EINVAL;
>
> - rcnt = info->read((char *)buf, len, zone->off);
> + rcnt = info->ops->read((char *)buf, len, zone->off);
> if (rcnt == -ENOMSG) {
> pr_debug("%s with id %lu may be broken, skip\n",
> zone->name, i);
> continue;
> } else if (rcnt != len) {
> - pr_err("read %s with id %lu failed\n", zone->name, i);
> + pr_err("read %s with id %lu failed\n", zone->name,
> + i);
> return (int)rcnt < 0 ? (int)rcnt : -EIO;
> }
>
> @@ -495,11 +496,11 @@ static int psz_recover_zone(struct psz_context *cxt, struct pstore_zone *zone)
> return 0;
> }
>
> - if (unlikely(!info->read))
> + if (unlikely(!info->ops->read))
> return -EINVAL;
>
> len = sizeof(struct psz_buffer);
> - rcnt = info->read((char *)&tmpbuf, len, zone->off);
> + rcnt = info->ops->read((char *)&tmpbuf, len, zone->off);
> if (rcnt != len) {
> pr_debug("read zone %s failed\n", zone->name);
> return (int)rcnt < 0 ? (int)rcnt : -EIO;
> @@ -541,7 +542,7 @@ static int psz_recover_zone(struct psz_context *cxt, struct pstore_zone *zone)
> off = zone->off + sizeof(*oldbuf);
>
> /* get part of data */
> - rcnt = info->read(buf, len - start, off + start);
> + rcnt = info->ops->read(buf, len - start, off + start);
> if (rcnt != len - start) {
> pr_err("read zone %s failed\n", zone->name);
> ret = (int)rcnt < 0 ? (int)rcnt : -EIO;
> @@ -549,7 +550,7 @@ static int psz_recover_zone(struct psz_context *cxt, struct pstore_zone *zone)
> }
>
> /* get the rest of data */
> - rcnt = info->read(buf + len - start, start, off);
> + rcnt = info->ops->read(buf + len - start, start, off);
> if (rcnt != start) {
> pr_err("read zone %s failed\n", zone->name);
> ret = (int)rcnt < 0 ? (int)rcnt : -EIO;
> @@ -671,8 +672,8 @@ static inline int psz_kmsg_erase(struct psz_context *cxt,
>
> size = buffer_datalen(zone) + sizeof(*zone->buffer);
> atomic_set(&zone->buffer->datalen, 0);
> - if (cxt->pstore_zone_info->erase)
> - return cxt->pstore_zone_info->erase(size, zone->off);
> + if (cxt->pstore_zone_info->ops->erase)
> + return cxt->pstore_zone_info->ops->erase(size, zone->off);
> else
> return psz_zone_write(zone, FLUSH_META, NULL, 0, 0);
> }
> @@ -1185,8 +1186,9 @@ static struct pstore_zone *psz_init_zone(enum pstore_type_id type,
>
> *off += size;
>
> - pr_debug("pszone %s: off 0x%llx, %zu header, %zu data\n", zone->name,
> - zone->off, sizeof(*zone->buffer), zone->buffer_size);
> + pr_debug("pszone %s: off 0x%llx, %zu header, %zu data\n",
> + zone->name, zone->off, sizeof(*zone->buffer),
> + zone->buffer_size);
> return zone;
> }
>
> @@ -1310,7 +1312,7 @@ int register_pstore_zone(struct pstore_zone_info *info)
> return -EINVAL;
> }
>
> - if (!info->name || !info->name[0])
> + if (!info->ops->name || !info->ops->name[0])
> return -EINVAL;
>
> #define check_size(name, size) { \
> @@ -1338,7 +1340,7 @@ int register_pstore_zone(struct pstore_zone_info *info)
> * if no @read, pstore may mount failed.
> * if no @write, pstore do not support to remove record file.
> */
> - if (!info->read || !info->write) {
> + if (!info->ops || !info->ops->read || !info->ops->write) {
> pr_err("no valid general read/write interface\n");
> return -EINVAL;
> }
> @@ -1346,13 +1348,13 @@ int register_pstore_zone(struct pstore_zone_info *info)
> mutex_lock(&cxt->pstore_zone_info_lock);
> if (cxt->pstore_zone_info) {
> pr_warn("'%s' already loaded: ignoring '%s'\n",
> - cxt->pstore_zone_info->name, info->name);
> + cxt->pstore_zone_info->ops->name, info->ops->name);
> mutex_unlock(&cxt->pstore_zone_info_lock);
> return -EBUSY;
> }
> cxt->pstore_zone_info = info;
>
> - pr_debug("register %s with properties:\n", info->name);
> + pr_debug("register %s with properties:\n", info->ops->name);
> pr_debug("\ttotal size : %ld Bytes\n", info->total_size);
> pr_debug("\tkmsg size : %ld Bytes\n", info->kmsg_size);
> pr_debug("\tpmsg size : %ld Bytes\n", info->pmsg_size);
> @@ -1376,14 +1378,14 @@ int register_pstore_zone(struct pstore_zone_info *info)
> }
> cxt->pstore.data = cxt;
>
> - pr_info("registered %s as backend for", info->name);
> + pr_info("registered %s as backend for", info->ops->name);
> cxt->pstore.max_reason = info->max_reason;
> - cxt->pstore.name = info->name;
> + cxt->pstore.name = info->ops->name;
> if (info->kmsg_size) {
> cxt->pstore.flags |= PSTORE_FLAGS_DMESG;
> pr_cont(" kmsg(%s",
> kmsg_dump_reason_str(cxt->pstore.max_reason));
> - if (cxt->pstore_zone_info->panic_write)
> + if (cxt->pstore_zone_info->ops->panic_write)
> pr_cont(",panic_write");
> pr_cont(")");
> }
> diff --git a/include/linux/pstore_blk.h b/include/linux/pstore_blk.h
> index 99564f93d77488..095a44ce5e122c 100644
> --- a/include/linux/pstore_blk.h
> +++ b/include/linux/pstore_blk.h
> @@ -15,31 +15,12 @@
> * @flags: Refer to macro starting with PSTORE_FLAGS defined in
> * linux/pstore.h. It means what front-ends this device support.
> * Zero means all backends for compatible.
> - * @read: The general read operation. Both of the function parameters
> - * @size and @offset are relative value to bock device (not the
> - * whole disk).
> - * On success, the number of bytes should be returned, others
> - * means error.
> - * @write: The same as @read, but the following error number:
> - * -EBUSY means try to write again later.
> - * -ENOMSG means to try next zone.
> - * @erase: The general erase operation for device with special removing
> - * job. Both of the function parameters @size and @offset are
> - * relative value to storage.
> - * Return 0 on success and others on failure.
> - * @panic_write:The write operation only used for panic case. It's optional
> - * if you do not care panic log. The parameters are relative
> - * value to storage.
> - * On success, the number of bytes should be returned, others
> - * excluding -ENOMSG mean error. -ENOMSG means to try next zone.
> + * @ops: operations to access the device.
> */
> struct pstore_device_info {
> unsigned long total_size;
> unsigned int flags;
> - pstore_zone_read_op read;
> - pstore_zone_write_op write;
> - pstore_zone_erase_op erase;
> - pstore_zone_write_op panic_write;
> + const struct pstore_zone_ops *ops;
> };
>
> int register_pstore_device(struct pstore_device_info *dev);
> diff --git a/include/linux/pstore_zone.h b/include/linux/pstore_zone.h
> index 1e35eaa33e5e0d..7bff124c96d8b1 100644
> --- a/include/linux/pstore_zone.h
> +++ b/include/linux/pstore_zone.h
> @@ -5,22 +5,10 @@
>
> #include <linux/types.h>
>
> -typedef ssize_t (*pstore_zone_read_op)(char *, size_t, loff_t);
> -typedef ssize_t (*pstore_zone_write_op)(const char *, size_t, loff_t);
> -typedef ssize_t (*pstore_zone_erase_op)(size_t, loff_t);
> /**
> - * struct pstore_zone_info - pstore/zone back-end driver structure
> + * struct pstore_zone_ops - pstore/zone ops structure
> *
> - * @owner: Module which is responsible for this back-end driver.
> * @name: Name of the back-end driver.
> - * @total_size: The total size in bytes pstore/zone can use. It must be greater
> - * than 4096 and be multiple of 4096.
> - * @kmsg_size: The size of oops/panic zone. Zero means disabled, otherwise,
> - * it must be multiple of SECTOR_SIZE(512 Bytes).
> - * @max_reason: Maximum kmsg dump reason to store.
> - * @pmsg_size: The size of pmsg zone which is the same as @kmsg_size.
> - * @console_size:The size of console zone which is the same as @kmsg_size.
> - * @ftrace_size:The size of ftrace zone which is the same as @kmsg_size.
> * @read: The general read operation. Both of the function parameters
> * @size and @offset are relative value to storage.
> * On success, the number of bytes should be returned, others
> @@ -38,20 +26,35 @@ typedef ssize_t (*pstore_zone_erase_op)(size_t, loff_t);
> * On success, the number of bytes should be returned, others
> * excluding -ENOMSG mean error. -ENOMSG means to try next zone.
> */
> -struct pstore_zone_info {
> - struct module *owner;
> +struct pstore_zone_ops {
> const char *name;
> + ssize_t (*read)(char *buf, size_t count, loff_t pos);
> + ssize_t (*write)(const char *buf, size_t bytes, loff_t pos);
> + ssize_t (*erase)(size_t byes, loff_t pos);
> + ssize_t (*panic_write)(const char *buf, size_t bytes, loff_t pos);
> +};
>
> +/**
> + * struct pstore_zone_info - pstore/zone back-end driver structure
> + *
> + * @ops: Operations to access the zone.
> + * @total_size: The total size in bytes pstore/zone can use. It must be greater
> + * than 4096 and be multiple of 4096.
> + * @kmsg_size: The size of oops/panic zone. Zero means disabled, otherwise,
> + * it must be multiple of SECTOR_SIZE(512 Bytes).
> + * @max_reason: Maximum kmsg dump reason to store.
> + * @pmsg_size: The size of pmsg zone which is the same as @kmsg_size.
> + * @console_size:The size of console zone which is the same as @kmsg_size.
> + * @ftrace_size:The size of ftrace zone which is the same as @kmsg_size.
> + */
> +struct pstore_zone_info {
> + const struct pstore_zone_ops *ops;
> unsigned long total_size;
> unsigned long kmsg_size;
> int max_reason;
> unsigned long pmsg_size;
> unsigned long console_size;
> unsigned long ftrace_size;
> - pstore_zone_read_op read;
> - pstore_zone_write_op write;
> - pstore_zone_erase_op erase;
> - pstore_zone_write_op panic_write;
> };
>
> extern int register_pstore_zone(struct pstore_zone_info *info);
> --
> 2.28.0
>
--
Kees Cook
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 7/9] pstore/blk: remove struct pstore_device_info
2020-10-16 13:20 simplify pstore-blk Christoph Hellwig
` (5 preceding siblings ...)
2020-10-16 13:20 ` [PATCH 6/9] pstore/zone: split struct pstore_zone_info Christoph Hellwig
@ 2020-10-16 13:20 ` Christoph Hellwig
2020-12-01 19:44 ` Kees Cook
2020-10-16 13:20 ` [PATCH 8/9] pstore/blk: use the normal block device I/O path Christoph Hellwig
` (3 subsequent siblings)
10 siblings, 1 reply; 28+ messages in thread
From: Christoph Hellwig @ 2020-10-16 13:20 UTC (permalink / raw)
To: Kees Cook, Anton Vorontsov, Colin Cross, Tony Luck, WeiXiong Liao
Cc: linux-mtd, linux-kernel
The total_size and flags are only needed at registrations time, so just
pass them to register_pstore_device directly.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
drivers/mtd/mtdpstore.c | 10 ++--
fs/pstore/blk.c | 98 ++++++++++++++++----------------------
include/linux/pstore_blk.h | 21 ++------
3 files changed, 47 insertions(+), 82 deletions(-)
diff --git a/drivers/mtd/mtdpstore.c b/drivers/mtd/mtdpstore.c
index 232ba5c39c2a55..88d0305ca27336 100644
--- a/drivers/mtd/mtdpstore.c
+++ b/drivers/mtd/mtdpstore.c
@@ -12,7 +12,6 @@
static struct mtdpstore_context {
int index;
struct pstore_blk_config info;
- struct pstore_device_info dev;
struct mtd_info *mtd;
unsigned long *rmmap; /* removed bit map */
unsigned long *usedmap; /* used bit map */
@@ -431,12 +430,9 @@ static void mtdpstore_notify_add(struct mtd_info *mtd)
longcnt = BITS_TO_LONGS(div_u64(mtd->size, mtd->erasesize));
cxt->badmap = kcalloc(longcnt, sizeof(long), GFP_KERNEL);
- cxt->dev.total_size = mtd->size;
/* just support dmesg right now */
- cxt->dev.flags = PSTORE_FLAGS_DMESG;
- cxt->dev.ops = &mtdpstore_ops;
-
- ret = register_pstore_device(&cxt->dev);
+ ret = register_pstore_device(&mtdpstore_ops, mtd->size,
+ PSTORE_FLAGS_DMESG);
if (ret) {
dev_err(&mtd->dev, "mtd%d register to psblk failed\n",
mtd->index);
@@ -531,7 +527,7 @@ static void mtdpstore_notify_remove(struct mtd_info *mtd)
mtdpstore_flush_removed(cxt);
- unregister_pstore_device(&cxt->dev);
+ unregister_pstore_device(&mtdpstore_ops);
kfree(cxt->badmap);
kfree(cxt->usedmap);
kfree(cxt->rmmap);
diff --git a/fs/pstore/blk.c b/fs/pstore/blk.c
index f7c7f325e42c71..0430b190a1df2a 100644
--- a/fs/pstore/blk.c
+++ b/fs/pstore/blk.c
@@ -102,27 +102,40 @@ static struct pstore_zone_info *pstore_zone_info;
_##name_; \
})
-static int __register_pstore_device(struct pstore_device_info *dev)
+/**
+ * register_pstore_device() - register device to pstore/blk
+ *
+ * @ops: operations to access the device.
+ * @total_size: The total size in bytes pstore/blk can use. It must be greater
+ * than 4096 and be multiple of 4096.
+ * @flags: Refer to macro starting with PSTORE_FLAGS defined in
+ * linux/pstore.h. It means what front-ends this device support.
+ * Zero means all backends for compatible.
+ */
+int register_pstore_device(const struct pstore_zone_ops *ops,
+ unsigned long total_size, unsigned int flags)
{
int ret;
- lockdep_assert_held(&pstore_blk_lock);
-
- if (!dev || !dev->total_size || !dev->ops ||
- !dev->ops->read || !dev->ops->write)
+ if (!ops || !ops->read || !ops->write || !total_size)
return -EINVAL;
/* someone already registered before */
- if (pstore_zone_info)
- return -EBUSY;
+ mutex_lock(&pstore_blk_lock);
+ if (pstore_zone_info) {
+ ret = -EBUSY;
+ goto out_unlock;
+ }
pstore_zone_info = kzalloc(sizeof(struct pstore_zone_info), GFP_KERNEL);
- if (!pstore_zone_info)
- return -ENOMEM;
+ if (!pstore_zone_info) {
+ ret = -ENOMEM;
+ goto out_unlock;
+ }
/* zero means not limit on which backends to attempt to store. */
- if (!dev->flags)
- dev->flags = UINT_MAX;
+ if (!flags)
+ flags = UINT_MAX;
#define verify_size(name, alignsize, enabled) { \
long _##name_; \
@@ -134,63 +147,40 @@ static int __register_pstore_device(struct pstore_device_info *dev)
pstore_zone_info->name = _##name_; \
}
- verify_size(kmsg_size, 4096, dev->flags & PSTORE_FLAGS_DMESG);
- verify_size(pmsg_size, 4096, dev->flags & PSTORE_FLAGS_PMSG);
- verify_size(console_size, 4096, dev->flags & PSTORE_FLAGS_CONSOLE);
- verify_size(ftrace_size, 4096, dev->flags & PSTORE_FLAGS_FTRACE);
+ verify_size(kmsg_size, 4096, flags & PSTORE_FLAGS_DMESG);
+ verify_size(pmsg_size, 4096, flags & PSTORE_FLAGS_PMSG);
+ verify_size(console_size, 4096, flags & PSTORE_FLAGS_CONSOLE);
+ verify_size(ftrace_size, 4096, flags & PSTORE_FLAGS_FTRACE);
#undef verify_size
- pstore_zone_info->total_size = dev->total_size;
+ pstore_zone_info->total_size = total_size;
pstore_zone_info->max_reason = max_reason;
- pstore_zone_info->ops = dev->ops;
+ pstore_zone_info->ops = ops;
ret = register_pstore_zone(pstore_zone_info);
if (ret) {
kfree(pstore_zone_info);
pstore_zone_info = NULL;
}
+out_unlock:
+ mutex_unlock(&pstore_blk_lock);
return ret;
}
+EXPORT_SYMBOL_GPL(register_pstore_device);
+
/**
- * register_pstore_device() - register non-block device to pstore/blk
- *
- * @dev: non-block device information
+ * unregister_pstore_device() - unregister a device from pstore/blk
*
- * Return:
- * * 0 - OK
- * * Others - something error.
+ * @ops: device operations
*/
-int register_pstore_device(struct pstore_device_info *dev)
+void unregister_pstore_device(const struct pstore_zone_ops *ops)
{
- int ret;
-
mutex_lock(&pstore_blk_lock);
- ret = __register_pstore_device(dev);
- mutex_unlock(&pstore_blk_lock);
-
- return ret;
-}
-EXPORT_SYMBOL_GPL(register_pstore_device);
-
-static void __unregister_pstore_device(struct pstore_device_info *dev)
-{
- lockdep_assert_held(&pstore_blk_lock);
- if (pstore_zone_info && pstore_zone_info->ops == dev->ops) {
+ if (pstore_zone_info && pstore_zone_info->ops == ops) {
unregister_pstore_zone(pstore_zone_info);
kfree(pstore_zone_info);
pstore_zone_info = NULL;
}
-}
-
-/**
- * unregister_pstore_device() - unregister non-block device from pstore/blk
- *
- * @dev: non-block device information
- */
-void unregister_pstore_device(struct pstore_device_info *dev)
-{
- mutex_lock(&pstore_blk_lock);
- __unregister_pstore_device(dev);
mutex_unlock(&pstore_blk_lock);
}
EXPORT_SYMBOL_GPL(unregister_pstore_device);
@@ -271,7 +261,6 @@ static int __init pstore_blk_init(void)
{
char bdev_name[BDEVNAME_SIZE];
struct block_device *bdev;
- struct pstore_device_info dev;
int ret = -ENODEV;
fmode_t mode = FMODE_READ | FMODE_WRITE | FMODE_EXCL;
sector_t nr_sects;
@@ -306,11 +295,8 @@ static int __init pstore_blk_init(void)
/* psblk_bdev must be assigned before register to pstore/blk */
psblk_bdev = bdev;
- memset(&dev, 0, sizeof(dev));
- dev.ops = &pstore_blk_zone_ops;
- dev.total_size = nr_sects << SECTOR_SHIFT;
-
- ret = register_pstore_device(&dev);
+ ret = register_pstore_device(&pstore_blk_zone_ops,
+ nr_sects << SECTOR_SHIFT, 0);
if (ret)
goto err_put_bdev;
@@ -327,11 +313,9 @@ late_initcall(pstore_blk_init);
static void __exit pstore_blk_exit(void)
{
- struct pstore_device_info dev = { .ops = &pstore_blk_zone_ops };
-
if (!psblk_bdev)
return;
- unregister_pstore_device(&dev);
+ unregister_pstore_device(&pstore_blk_zone_ops);
blkdev_put(psblk_bdev, FMODE_READ | FMODE_WRITE | FMODE_EXCL);
}
module_exit(pstore_blk_exit);
diff --git a/include/linux/pstore_blk.h b/include/linux/pstore_blk.h
index 095a44ce5e122c..0abd412a6cb3e3 100644
--- a/include/linux/pstore_blk.h
+++ b/include/linux/pstore_blk.h
@@ -7,24 +7,9 @@
#include <linux/pstore.h>
#include <linux/pstore_zone.h>
-/**
- * struct pstore_device_info - back-end pstore/blk driver structure.
- *
- * @total_size: The total size in bytes pstore/blk can use. It must be greater
- * than 4096 and be multiple of 4096.
- * @flags: Refer to macro starting with PSTORE_FLAGS defined in
- * linux/pstore.h. It means what front-ends this device support.
- * Zero means all backends for compatible.
- * @ops: operations to access the device.
- */
-struct pstore_device_info {
- unsigned long total_size;
- unsigned int flags;
- const struct pstore_zone_ops *ops;
-};
-
-int register_pstore_device(struct pstore_device_info *dev);
-void unregister_pstore_device(struct pstore_device_info *dev);
+int register_pstore_device(const struct pstore_zone_ops *ops,
+ unsigned long total_size, unsigned int flags);
+void unregister_pstore_device(const struct pstore_zone_ops *ops);
/**
* struct pstore_blk_config - the pstore_blk backend configuration
--
2.28.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 7/9] pstore/blk: remove struct pstore_device_info
2020-10-16 13:20 ` [PATCH 7/9] pstore/blk: remove struct pstore_device_info Christoph Hellwig
@ 2020-12-01 19:44 ` Kees Cook
0 siblings, 0 replies; 28+ messages in thread
From: Kees Cook @ 2020-12-01 19:44 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Anton Vorontsov, Colin Cross, Tony Luck, WeiXiong Liao,
linux-mtd, linux-kernel
On Fri, Oct 16, 2020 at 03:20:45PM +0200, Christoph Hellwig wrote:
> The total_size and flags are only needed at registrations time, so just
> pass them to register_pstore_device directly.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
Full NAK on this -- pstore was mess of function argument passing, and I
vowed to pass everything in structures after I finally cleaned all of
that up. I don't want anything that looks like this getting back into
the pstore code.
-Kees
> ---
> drivers/mtd/mtdpstore.c | 10 ++--
> fs/pstore/blk.c | 98 ++++++++++++++++----------------------
> include/linux/pstore_blk.h | 21 ++------
> 3 files changed, 47 insertions(+), 82 deletions(-)
>
> diff --git a/drivers/mtd/mtdpstore.c b/drivers/mtd/mtdpstore.c
> index 232ba5c39c2a55..88d0305ca27336 100644
> --- a/drivers/mtd/mtdpstore.c
> +++ b/drivers/mtd/mtdpstore.c
> @@ -12,7 +12,6 @@
> static struct mtdpstore_context {
> int index;
> struct pstore_blk_config info;
> - struct pstore_device_info dev;
> struct mtd_info *mtd;
> unsigned long *rmmap; /* removed bit map */
> unsigned long *usedmap; /* used bit map */
> @@ -431,12 +430,9 @@ static void mtdpstore_notify_add(struct mtd_info *mtd)
> longcnt = BITS_TO_LONGS(div_u64(mtd->size, mtd->erasesize));
> cxt->badmap = kcalloc(longcnt, sizeof(long), GFP_KERNEL);
>
> - cxt->dev.total_size = mtd->size;
> /* just support dmesg right now */
> - cxt->dev.flags = PSTORE_FLAGS_DMESG;
> - cxt->dev.ops = &mtdpstore_ops;
> -
> - ret = register_pstore_device(&cxt->dev);
> + ret = register_pstore_device(&mtdpstore_ops, mtd->size,
> + PSTORE_FLAGS_DMESG);
> if (ret) {
> dev_err(&mtd->dev, "mtd%d register to psblk failed\n",
> mtd->index);
> @@ -531,7 +527,7 @@ static void mtdpstore_notify_remove(struct mtd_info *mtd)
>
> mtdpstore_flush_removed(cxt);
>
> - unregister_pstore_device(&cxt->dev);
> + unregister_pstore_device(&mtdpstore_ops);
> kfree(cxt->badmap);
> kfree(cxt->usedmap);
> kfree(cxt->rmmap);
> diff --git a/fs/pstore/blk.c b/fs/pstore/blk.c
> index f7c7f325e42c71..0430b190a1df2a 100644
> --- a/fs/pstore/blk.c
> +++ b/fs/pstore/blk.c
> @@ -102,27 +102,40 @@ static struct pstore_zone_info *pstore_zone_info;
> _##name_; \
> })
>
> -static int __register_pstore_device(struct pstore_device_info *dev)
> +/**
> + * register_pstore_device() - register device to pstore/blk
> + *
> + * @ops: operations to access the device.
> + * @total_size: The total size in bytes pstore/blk can use. It must be greater
> + * than 4096 and be multiple of 4096.
> + * @flags: Refer to macro starting with PSTORE_FLAGS defined in
> + * linux/pstore.h. It means what front-ends this device support.
> + * Zero means all backends for compatible.
> + */
> +int register_pstore_device(const struct pstore_zone_ops *ops,
> + unsigned long total_size, unsigned int flags)
> {
> int ret;
>
> - lockdep_assert_held(&pstore_blk_lock);
> -
> - if (!dev || !dev->total_size || !dev->ops ||
> - !dev->ops->read || !dev->ops->write)
> + if (!ops || !ops->read || !ops->write || !total_size)
> return -EINVAL;
>
> /* someone already registered before */
> - if (pstore_zone_info)
> - return -EBUSY;
> + mutex_lock(&pstore_blk_lock);
> + if (pstore_zone_info) {
> + ret = -EBUSY;
> + goto out_unlock;
> + }
>
> pstore_zone_info = kzalloc(sizeof(struct pstore_zone_info), GFP_KERNEL);
> - if (!pstore_zone_info)
> - return -ENOMEM;
> + if (!pstore_zone_info) {
> + ret = -ENOMEM;
> + goto out_unlock;
> + }
>
> /* zero means not limit on which backends to attempt to store. */
> - if (!dev->flags)
> - dev->flags = UINT_MAX;
> + if (!flags)
> + flags = UINT_MAX;
>
> #define verify_size(name, alignsize, enabled) { \
> long _##name_; \
> @@ -134,63 +147,40 @@ static int __register_pstore_device(struct pstore_device_info *dev)
> pstore_zone_info->name = _##name_; \
> }
>
> - verify_size(kmsg_size, 4096, dev->flags & PSTORE_FLAGS_DMESG);
> - verify_size(pmsg_size, 4096, dev->flags & PSTORE_FLAGS_PMSG);
> - verify_size(console_size, 4096, dev->flags & PSTORE_FLAGS_CONSOLE);
> - verify_size(ftrace_size, 4096, dev->flags & PSTORE_FLAGS_FTRACE);
> + verify_size(kmsg_size, 4096, flags & PSTORE_FLAGS_DMESG);
> + verify_size(pmsg_size, 4096, flags & PSTORE_FLAGS_PMSG);
> + verify_size(console_size, 4096, flags & PSTORE_FLAGS_CONSOLE);
> + verify_size(ftrace_size, 4096, flags & PSTORE_FLAGS_FTRACE);
> #undef verify_size
>
> - pstore_zone_info->total_size = dev->total_size;
> + pstore_zone_info->total_size = total_size;
> pstore_zone_info->max_reason = max_reason;
> - pstore_zone_info->ops = dev->ops;
> + pstore_zone_info->ops = ops;
>
> ret = register_pstore_zone(pstore_zone_info);
> if (ret) {
> kfree(pstore_zone_info);
> pstore_zone_info = NULL;
> }
> +out_unlock:
> + mutex_unlock(&pstore_blk_lock);
> return ret;
> }
> +EXPORT_SYMBOL_GPL(register_pstore_device);
> +
> /**
> - * register_pstore_device() - register non-block device to pstore/blk
> - *
> - * @dev: non-block device information
> + * unregister_pstore_device() - unregister a device from pstore/blk
> *
> - * Return:
> - * * 0 - OK
> - * * Others - something error.
> + * @ops: device operations
> */
> -int register_pstore_device(struct pstore_device_info *dev)
> +void unregister_pstore_device(const struct pstore_zone_ops *ops)
> {
> - int ret;
> -
> mutex_lock(&pstore_blk_lock);
> - ret = __register_pstore_device(dev);
> - mutex_unlock(&pstore_blk_lock);
> -
> - return ret;
> -}
> -EXPORT_SYMBOL_GPL(register_pstore_device);
> -
> -static void __unregister_pstore_device(struct pstore_device_info *dev)
> -{
> - lockdep_assert_held(&pstore_blk_lock);
> - if (pstore_zone_info && pstore_zone_info->ops == dev->ops) {
> + if (pstore_zone_info && pstore_zone_info->ops == ops) {
> unregister_pstore_zone(pstore_zone_info);
> kfree(pstore_zone_info);
> pstore_zone_info = NULL;
> }
> -}
> -
> -/**
> - * unregister_pstore_device() - unregister non-block device from pstore/blk
> - *
> - * @dev: non-block device information
> - */
> -void unregister_pstore_device(struct pstore_device_info *dev)
> -{
> - mutex_lock(&pstore_blk_lock);
> - __unregister_pstore_device(dev);
> mutex_unlock(&pstore_blk_lock);
> }
> EXPORT_SYMBOL_GPL(unregister_pstore_device);
> @@ -271,7 +261,6 @@ static int __init pstore_blk_init(void)
> {
> char bdev_name[BDEVNAME_SIZE];
> struct block_device *bdev;
> - struct pstore_device_info dev;
> int ret = -ENODEV;
> fmode_t mode = FMODE_READ | FMODE_WRITE | FMODE_EXCL;
> sector_t nr_sects;
> @@ -306,11 +295,8 @@ static int __init pstore_blk_init(void)
> /* psblk_bdev must be assigned before register to pstore/blk */
> psblk_bdev = bdev;
>
> - memset(&dev, 0, sizeof(dev));
> - dev.ops = &pstore_blk_zone_ops;
> - dev.total_size = nr_sects << SECTOR_SHIFT;
> -
> - ret = register_pstore_device(&dev);
> + ret = register_pstore_device(&pstore_blk_zone_ops,
> + nr_sects << SECTOR_SHIFT, 0);
> if (ret)
> goto err_put_bdev;
>
> @@ -327,11 +313,9 @@ late_initcall(pstore_blk_init);
>
> static void __exit pstore_blk_exit(void)
> {
> - struct pstore_device_info dev = { .ops = &pstore_blk_zone_ops };
> -
> if (!psblk_bdev)
> return;
> - unregister_pstore_device(&dev);
> + unregister_pstore_device(&pstore_blk_zone_ops);
> blkdev_put(psblk_bdev, FMODE_READ | FMODE_WRITE | FMODE_EXCL);
> }
> module_exit(pstore_blk_exit);
> diff --git a/include/linux/pstore_blk.h b/include/linux/pstore_blk.h
> index 095a44ce5e122c..0abd412a6cb3e3 100644
> --- a/include/linux/pstore_blk.h
> +++ b/include/linux/pstore_blk.h
> @@ -7,24 +7,9 @@
> #include <linux/pstore.h>
> #include <linux/pstore_zone.h>
>
> -/**
> - * struct pstore_device_info - back-end pstore/blk driver structure.
> - *
> - * @total_size: The total size in bytes pstore/blk can use. It must be greater
> - * than 4096 and be multiple of 4096.
> - * @flags: Refer to macro starting with PSTORE_FLAGS defined in
> - * linux/pstore.h. It means what front-ends this device support.
> - * Zero means all backends for compatible.
> - * @ops: operations to access the device.
> - */
> -struct pstore_device_info {
> - unsigned long total_size;
> - unsigned int flags;
> - const struct pstore_zone_ops *ops;
> -};
> -
> -int register_pstore_device(struct pstore_device_info *dev);
> -void unregister_pstore_device(struct pstore_device_info *dev);
> +int register_pstore_device(const struct pstore_zone_ops *ops,
> + unsigned long total_size, unsigned int flags);
> +void unregister_pstore_device(const struct pstore_zone_ops *ops);
>
> /**
> * struct pstore_blk_config - the pstore_blk backend configuration
> --
> 2.28.0
>
--
Kees Cook
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 8/9] pstore/blk: use the normal block device I/O path
2020-10-16 13:20 simplify pstore-blk Christoph Hellwig
` (6 preceding siblings ...)
2020-10-16 13:20 ` [PATCH 7/9] pstore/blk: remove struct pstore_device_info Christoph Hellwig
@ 2020-10-16 13:20 ` Christoph Hellwig
2020-11-08 14:43 ` 廖威雄
2020-12-01 19:52 ` Kees Cook
2020-10-16 13:20 ` [PATCH 9/9] pstore/blk: don't depend on CONFIG_BLOCK Christoph Hellwig
` (2 subsequent siblings)
10 siblings, 2 replies; 28+ messages in thread
From: Christoph Hellwig @ 2020-10-16 13:20 UTC (permalink / raw)
To: Kees Cook, Anton Vorontsov, Colin Cross, Tony Luck, WeiXiong Liao
Cc: linux-mtd, linux-kernel
Stop poking into block layer internals and just open the block device
file an use kernel_read and kernel_write on it. Note that this means
the transformation from name_to_dev_t can't be used anymore, and proper
block device file names must be used.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/pstore/blk.c | 152 +++++++++++++------------------------
include/linux/pstore_blk.h | 2 +
init/main.c | 4 +
3 files changed, 57 insertions(+), 101 deletions(-)
diff --git a/fs/pstore/blk.c b/fs/pstore/blk.c
index 0430b190a1df2a..bd4eadfc9bd795 100644
--- a/fs/pstore/blk.c
+++ b/fs/pstore/blk.c
@@ -8,15 +8,13 @@
#include <linux/kernel.h>
#include <linux/module.h>
-#include "../../block/blk.h"
#include <linux/blkdev.h>
#include <linux/string.h>
-#include <linux/of.h>
-#include <linux/of_address.h>
-#include <linux/platform_device.h>
#include <linux/pstore_blk.h>
+#include <linux/fs.h>
+#include <linux/file.h>
+#include <linux/init_syscalls.h>
#include <linux/mount.h>
-#include <linux/uio.h>
static long kmsg_size = CONFIG_PSTORE_BLK_KMSG_SIZE;
module_param(kmsg_size, long, 0400);
@@ -88,7 +86,6 @@ MODULE_PARM_DESC(blkdev, "block device for pstore storage");
* during the register/unregister functions.
*/
static DEFINE_MUTEX(pstore_blk_lock);
-static struct block_device *psblk_bdev;
static struct pstore_zone_info *pstore_zone_info;
#define check_size(name, alignsize) ({ \
@@ -185,70 +182,20 @@ void unregister_pstore_device(const struct pstore_zone_ops *ops)
}
EXPORT_SYMBOL_GPL(unregister_pstore_device);
+static struct file *psblk_file;
+
static ssize_t psblk_generic_blk_read(char *buf, size_t bytes, loff_t pos)
{
- struct block_device *bdev = psblk_bdev;
- struct file file;
- struct kiocb kiocb;
- struct iov_iter iter;
- struct kvec iov = {.iov_base = buf, .iov_len = bytes};
-
- if (!bdev)
- return -ENODEV;
-
- memset(&file, 0, sizeof(struct file));
- file.f_mapping = bdev->bd_inode->i_mapping;
- file.f_flags = O_DSYNC | __O_SYNC | O_NOATIME;
- file.f_inode = bdev->bd_inode;
- file_ra_state_init(&file.f_ra, file.f_mapping);
-
- init_sync_kiocb(&kiocb, &file);
- kiocb.ki_pos = pos;
- iov_iter_kvec(&iter, READ, &iov, 1, bytes);
-
- return generic_file_read_iter(&kiocb, &iter);
+ return kernel_read(psblk_file, buf, bytes, &pos);
}
static ssize_t psblk_generic_blk_write(const char *buf, size_t bytes,
loff_t pos)
{
- struct block_device *bdev = psblk_bdev;
- struct iov_iter iter;
- struct kiocb kiocb;
- struct file file;
- ssize_t ret;
- struct kvec iov = {.iov_base = (void *)buf, .iov_len = bytes};
-
- if (!bdev)
- return -ENODEV;
-
/* Console/Ftrace backend may handle buffer until flush dirty zones */
if (in_interrupt() || irqs_disabled())
return -EBUSY;
-
- memset(&file, 0, sizeof(struct file));
- file.f_mapping = bdev->bd_inode->i_mapping;
- file.f_flags = O_DSYNC | __O_SYNC | O_NOATIME;
- file.f_inode = bdev->bd_inode;
-
- init_sync_kiocb(&kiocb, &file);
- kiocb.ki_pos = pos;
- iov_iter_kvec(&iter, WRITE, &iov, 1, bytes);
-
- inode_lock(bdev->bd_inode);
- ret = generic_write_checks(&kiocb, &iter);
- if (ret > 0)
- ret = generic_perform_write(&file, &iter, pos);
- inode_unlock(bdev->bd_inode);
-
- if (likely(ret > 0)) {
- const struct file_operations f_op = {.fsync = blkdev_fsync};
-
- file.f_op = &f_op;
- kiocb.ki_pos += ret;
- ret = generic_write_sync(&kiocb, ret);
- }
- return ret;
+ return kernel_write(psblk_file, buf, bytes, &pos);
}
static const struct pstore_zone_ops pstore_blk_zone_ops = {
@@ -257,68 +204,71 @@ static const struct pstore_zone_ops pstore_blk_zone_ops = {
.write = psblk_generic_blk_write,
};
-static int __init pstore_blk_init(void)
+static int __init __pstore_blk_init(const char *name)
{
- char bdev_name[BDEVNAME_SIZE];
- struct block_device *bdev;
- int ret = -ENODEV;
- fmode_t mode = FMODE_READ | FMODE_WRITE | FMODE_EXCL;
- sector_t nr_sects;
-
+ int ret = -EINVAL;
+
if (!best_effort || !blkdev[0])
return 0;
- /* hold bdev exclusively */
- bdev = blkdev_get_by_path(blkdev, mode, blkdev);
- if (IS_ERR(bdev)) {
- dev_t devt;
-
- devt = name_to_dev_t(blkdev);
- if (devt == 0) {
- pr_err("failed to open '%s'!\n", blkdev);
- return -ENODEV;
- }
- bdev = blkdev_get_by_dev(devt, mode, blkdev);
- if (IS_ERR(bdev)) {
- pr_err("failed to open '%s'!\n", blkdev);
- return PTR_ERR(bdev);
- }
+ psblk_file = filp_open(name, O_RDWR | O_DSYNC | O_NOATIME | O_EXCL, 0);
+ if (IS_ERR(psblk_file)) {
+ ret = PTR_ERR(psblk_file);
+ pr_err("failed to open '%s': %d!\n", name, ret);
+ goto out;
}
-
- nr_sects = part_nr_sects_read(bdev->bd_part);
- if (!nr_sects) {
- pr_err("not enough space for '%s'\n", blkdev);
- ret = -ENOSPC;
- goto err_put_bdev;
+ if (!S_ISBLK(file_inode(psblk_file)->i_mode)) {
+ pr_err("'%s' is not block device!\n", blkdev);
+ goto out_fput;
}
- /* psblk_bdev must be assigned before register to pstore/blk */
- psblk_bdev = bdev;
-
ret = register_pstore_device(&pstore_blk_zone_ops,
- nr_sects << SECTOR_SHIFT, 0);
+ file_inode(psblk_file)->i_bdev->bd_inode->i_size, 0);
if (ret)
- goto err_put_bdev;
+ goto out_fput;
- bdevname(bdev, bdev_name);
- pr_info("attached %s (no dedicated panic_write!)\n", bdev_name);
+ pr_info("using device '%s'\n", blkdev);
return 0;
-
-err_put_bdev:
- blkdev_put(bdev, FMODE_READ | FMODE_WRITE | FMODE_EXCL);
- psblk_bdev = NULL;
+out_fput:
+ fput(psblk_file);
+out:
+ psblk_file = NULL;
return ret;
}
+
+#ifdef MODULE
+static int __init pstore_blk_init(void)
+{
+ return __pstore_blk_init(blkdev);
+}
late_initcall(pstore_blk_init);
static void __exit pstore_blk_exit(void)
{
- if (!psblk_bdev)
+ if (!psblk_file)
return;
unregister_pstore_device(&pstore_blk_zone_ops);
- blkdev_put(psblk_bdev, FMODE_READ | FMODE_WRITE | FMODE_EXCL);
+ fput(psblk_file);
}
module_exit(pstore_blk_exit);
+#else /* MODULE */
+/*
+ * During early boot the real root file system hasn't been mounted yet,
+ * and not device nodes are present yet. Use the same scheme to find
+ * the device that we use for mounting the root file system.
+ */
+void __init pstore_blk_early_init(void)
+{
+ const char devname[] = "/dev/pstore-blk";
+ dev_t dev = name_to_dev_t(blkdev);
+
+ if (!dev)
+ return;
+ init_unlink(devname);
+ init_mknod(devname, S_IFBLK | 0600, new_encode_dev(dev));
+ __pstore_blk_init(devname);
+}
+#endif /* MODULE */
/* get information of pstore/blk */
int pstore_blk_get_config(struct pstore_blk_config *info)
diff --git a/include/linux/pstore_blk.h b/include/linux/pstore_blk.h
index 0abd412a6cb3e3..7c06b9d6740e2a 100644
--- a/include/linux/pstore_blk.h
+++ b/include/linux/pstore_blk.h
@@ -39,4 +39,6 @@ struct pstore_blk_config {
*/
int pstore_blk_get_config(struct pstore_blk_config *info);
+void __init pstore_blk_early_init(void);
+
#endif
diff --git a/init/main.c b/init/main.c
index 1af84337cb18d5..058cce64f70390 100644
--- a/init/main.c
+++ b/init/main.c
@@ -98,6 +98,7 @@
#include <linux/mem_encrypt.h>
#include <linux/kcsan.h>
#include <linux/init_syscalls.h>
+#include <linux/pstore_blk.h>
#include <asm/io.h>
#include <asm/bugs.h>
@@ -1524,6 +1525,9 @@ static noinline void __init kernel_init_freeable(void)
prepare_namespace();
}
+ if (IS_BUILTIN(CONFIG_PSTORE_BLK))
+ pstore_blk_early_init();
+
/*
* Ok, we have completed the initial bootup, and
* we're essentially up and running. Get rid of the
--
2.28.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 8/9] pstore/blk: use the normal block device I/O path
2020-10-16 13:20 ` [PATCH 8/9] pstore/blk: use the normal block device I/O path Christoph Hellwig
@ 2020-11-08 14:43 ` 廖威雄
2020-11-23 14:49 ` Christoph Hellwig
2020-12-01 19:52 ` Kees Cook
1 sibling, 1 reply; 28+ messages in thread
From: 廖威雄 @ 2020-11-08 14:43 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Kees Cook, Anton Vorontsov, Colin Cross, Tony Luck,
WeiXiong Liao, linux-mtd, linux-kernel
On Fri, Oct 16, 2020 at 11:33 PM Christoph Hellwig <hch@lst.de> wrote:
>
> Stop poking into block layer internals and just open the block device
> file an use kernel_read and kernel_write on it. Note that this means
> the transformation from name_to_dev_t can't be used anymore, and proper
> block device file names must be used.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> ---
> fs/pstore/blk.c | 152 +++++++++++++------------------------
> include/linux/pstore_blk.h | 2 +
> init/main.c | 4 +
> 3 files changed, 57 insertions(+), 101 deletions(-)
>
> diff --git a/fs/pstore/blk.c b/fs/pstore/blk.c
> index 0430b190a1df2a..bd4eadfc9bd795 100644
> --- a/fs/pstore/blk.c
> +++ b/fs/pstore/blk.c
> @@ -8,15 +8,13 @@
>
> #include <linux/kernel.h>
> #include <linux/module.h>
> -#include "../../block/blk.h"
> #include <linux/blkdev.h>
> #include <linux/string.h>
> -#include <linux/of.h>
> -#include <linux/of_address.h>
> -#include <linux/platform_device.h>
> #include <linux/pstore_blk.h>
> +#include <linux/fs.h>
> +#include <linux/file.h>
> +#include <linux/init_syscalls.h>
> #include <linux/mount.h>
> -#include <linux/uio.h>
>
> static long kmsg_size = CONFIG_PSTORE_BLK_KMSG_SIZE;
> module_param(kmsg_size, long, 0400);
> @@ -88,7 +86,6 @@ MODULE_PARM_DESC(blkdev, "block device for pstore storage");
> * during the register/unregister functions.
> */
> static DEFINE_MUTEX(pstore_blk_lock);
> -static struct block_device *psblk_bdev;
> static struct pstore_zone_info *pstore_zone_info;
>
> #define check_size(name, alignsize) ({ \
> @@ -185,70 +182,20 @@ void unregister_pstore_device(const struct pstore_zone_ops *ops)
> }
> EXPORT_SYMBOL_GPL(unregister_pstore_device);
>
> +static struct file *psblk_file;
> +
> static ssize_t psblk_generic_blk_read(char *buf, size_t bytes, loff_t pos)
> {
> - struct block_device *bdev = psblk_bdev;
> - struct file file;
> - struct kiocb kiocb;
> - struct iov_iter iter;
> - struct kvec iov = {.iov_base = buf, .iov_len = bytes};
> -
> - if (!bdev)
> - return -ENODEV;
> -
> - memset(&file, 0, sizeof(struct file));
> - file.f_mapping = bdev->bd_inode->i_mapping;
> - file.f_flags = O_DSYNC | __O_SYNC | O_NOATIME;
> - file.f_inode = bdev->bd_inode;
> - file_ra_state_init(&file.f_ra, file.f_mapping);
> -
> - init_sync_kiocb(&kiocb, &file);
> - kiocb.ki_pos = pos;
> - iov_iter_kvec(&iter, READ, &iov, 1, bytes);
> -
> - return generic_file_read_iter(&kiocb, &iter);
> + return kernel_read(psblk_file, buf, bytes, &pos);
> }
>
> static ssize_t psblk_generic_blk_write(const char *buf, size_t bytes,
> loff_t pos)
> {
> - struct block_device *bdev = psblk_bdev;
> - struct iov_iter iter;
> - struct kiocb kiocb;
> - struct file file;
> - ssize_t ret;
> - struct kvec iov = {.iov_base = (void *)buf, .iov_len = bytes};
> -
> - if (!bdev)
> - return -ENODEV;
> -
> /* Console/Ftrace backend may handle buffer until flush dirty zones */
> if (in_interrupt() || irqs_disabled())
> return -EBUSY;
> -
> - memset(&file, 0, sizeof(struct file));
> - file.f_mapping = bdev->bd_inode->i_mapping;
> - file.f_flags = O_DSYNC | __O_SYNC | O_NOATIME;
> - file.f_inode = bdev->bd_inode;
> -
> - init_sync_kiocb(&kiocb, &file);
> - kiocb.ki_pos = pos;
> - iov_iter_kvec(&iter, WRITE, &iov, 1, bytes);
> -
> - inode_lock(bdev->bd_inode);
> - ret = generic_write_checks(&kiocb, &iter);
> - if (ret > 0)
> - ret = generic_perform_write(&file, &iter, pos);
> - inode_unlock(bdev->bd_inode);
> -
> - if (likely(ret > 0)) {
> - const struct file_operations f_op = {.fsync = blkdev_fsync};
> -
> - file.f_op = &f_op;
> - kiocb.ki_pos += ret;
> - ret = generic_write_sync(&kiocb, ret);
> - }
> - return ret;
> + return kernel_write(psblk_file, buf, bytes, &pos);
> }
>
> static const struct pstore_zone_ops pstore_blk_zone_ops = {
> @@ -257,68 +204,71 @@ static const struct pstore_zone_ops pstore_blk_zone_ops = {
> .write = psblk_generic_blk_write,
> };
>
> -static int __init pstore_blk_init(void)
> +static int __init __pstore_blk_init(const char *name)
> {
> - char bdev_name[BDEVNAME_SIZE];
> - struct block_device *bdev;
> - int ret = -ENODEV;
> - fmode_t mode = FMODE_READ | FMODE_WRITE | FMODE_EXCL;
> - sector_t nr_sects;
> -
> + int ret = -EINVAL;
> +
> if (!best_effort || !blkdev[0])
> return 0;
>
> - /* hold bdev exclusively */
> - bdev = blkdev_get_by_path(blkdev, mode, blkdev);
> - if (IS_ERR(bdev)) {
> - dev_t devt;
> -
> - devt = name_to_dev_t(blkdev);
> - if (devt == 0) {
> - pr_err("failed to open '%s'!\n", blkdev);
> - return -ENODEV;
> - }
> - bdev = blkdev_get_by_dev(devt, mode, blkdev);
> - if (IS_ERR(bdev)) {
> - pr_err("failed to open '%s'!\n", blkdev);
> - return PTR_ERR(bdev);
> - }
> + psblk_file = filp_open(name, O_RDWR | O_DSYNC | O_NOATIME | O_EXCL, 0);
> + if (IS_ERR(psblk_file)) {
> + ret = PTR_ERR(psblk_file);
> + pr_err("failed to open '%s': %d!\n", name, ret);
> + goto out;
> }
> -
> - nr_sects = part_nr_sects_read(bdev->bd_part);
> - if (!nr_sects) {
> - pr_err("not enough space for '%s'\n", blkdev);
> - ret = -ENOSPC;
> - goto err_put_bdev;
> + if (!S_ISBLK(file_inode(psblk_file)->i_mode)) {
> + pr_err("'%s' is not block device!\n", blkdev);
> + goto out_fput;
> }
>
> - /* psblk_bdev must be assigned before register to pstore/blk */
> - psblk_bdev = bdev;
> -
> ret = register_pstore_device(&pstore_blk_zone_ops,
> - nr_sects << SECTOR_SHIFT, 0);
> + file_inode(psblk_file)->i_bdev->bd_inode->i_size, 0);
> if (ret)
> - goto err_put_bdev;
> + goto out_fput;
>
> - bdevname(bdev, bdev_name);
> - pr_info("attached %s (no dedicated panic_write!)\n", bdev_name);
> + pr_info("using device '%s'\n", blkdev);
> return 0;
> -
> -err_put_bdev:
> - blkdev_put(bdev, FMODE_READ | FMODE_WRITE | FMODE_EXCL);
> - psblk_bdev = NULL;
> +out_fput:
> + fput(psblk_file);
> +out:
> + psblk_file = NULL;
> return ret;
> }
> +
> +#ifdef MODULE
> +static int __init pstore_blk_init(void)
> +{
> + return __pstore_blk_init(blkdev);
> +}
> late_initcall(pstore_blk_init);
>
> static void __exit pstore_blk_exit(void)
> {
> - if (!psblk_bdev)
> + if (!psblk_file)
> return;
> unregister_pstore_device(&pstore_blk_zone_ops);
> - blkdev_put(psblk_bdev, FMODE_READ | FMODE_WRITE | FMODE_EXCL);
> + fput(psblk_file);
> }
> module_exit(pstore_blk_exit);
> +#else /* MODULE */
> +/*
> + * During early boot the real root file system hasn't been mounted yet,
> + * and not device nodes are present yet. Use the same scheme to find
> + * the device that we use for mounting the root file system.
> + */
> +void __init pstore_blk_early_init(void)
> +{
> + const char devname[] = "/dev/pstore-blk";
> + dev_t dev = name_to_dev_t(blkdev);
> +
> + if (!dev)
> + return;
> + init_unlink(devname);
> + init_mknod(devname, S_IFBLK | 0600, new_encode_dev(dev));
> + __pstore_blk_init(devname);
How about pstore/blk making /dev/pstore-blk node no matter build-in
or module. Then pstore/blk always opens /dev/pstore-blk. By this way,
blkdev can also be a device number if pstore/blk built as a module.
> +}
> +#endif /* MODULE */
>
> /* get information of pstore/blk */
> int pstore_blk_get_config(struct pstore_blk_config *info)
> diff --git a/include/linux/pstore_blk.h b/include/linux/pstore_blk.h
> index 0abd412a6cb3e3..7c06b9d6740e2a 100644
> --- a/include/linux/pstore_blk.h
> +++ b/include/linux/pstore_blk.h
> @@ -39,4 +39,6 @@ struct pstore_blk_config {
> */
> int pstore_blk_get_config(struct pstore_blk_config *info);
>
> +void __init pstore_blk_early_init(void);
> +
> #endif
> diff --git a/init/main.c b/init/main.c
> index 1af84337cb18d5..058cce64f70390 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -98,6 +98,7 @@
> #include <linux/mem_encrypt.h>
> #include <linux/kcsan.h>
> #include <linux/init_syscalls.h>
> +#include <linux/pstore_blk.h>
>
> #include <asm/io.h>
> #include <asm/bugs.h>
> @@ -1524,6 +1525,9 @@ static noinline void __init kernel_init_freeable(void)
> prepare_namespace();
> }
>
> + if (IS_BUILTIN(CONFIG_PSTORE_BLK))
> + pstore_blk_early_init();
> +
> /*
> * Ok, we have completed the initial bootup, and
> * we're essentially up and running. Get rid of the
> --
> 2.28.0
>
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 8/9] pstore/blk: use the normal block device I/O path
2020-11-08 14:43 ` 廖威雄
@ 2020-11-23 14:49 ` Christoph Hellwig
0 siblings, 0 replies; 28+ messages in thread
From: Christoph Hellwig @ 2020-11-23 14:49 UTC (permalink / raw)
To: 廖威雄
Cc: Christoph Hellwig, Kees Cook, Anton Vorontsov, Colin Cross,
Tony Luck, WeiXiong Liao, linux-mtd, linux-kernel
On Sun, Nov 08, 2020 at 10:43:29PM +0800, 廖威雄 wrote:
> > + const char devname[] = "/dev/pstore-blk";
> > + dev_t dev = name_to_dev_t(blkdev);
> > +
> > + if (!dev)
> > + return;
> > + init_unlink(devname);
> > + init_mknod(devname, S_IFBLK | 0600, new_encode_dev(dev));
> > + __pstore_blk_init(devname);
>
> How about pstore/blk making /dev/pstore-blk node no matter build-in
> or module. Then pstore/blk always opens /dev/pstore-blk. By this way,
> blkdev can also be a device number if pstore/blk built as a module.
Please read the comment above blkdev_get_by_dev on why no one should
add new interfaces based on the device number.
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: [PATCH 8/9] pstore/blk: use the normal block device I/O path
2020-10-16 13:20 ` [PATCH 8/9] pstore/blk: use the normal block device I/O path Christoph Hellwig
2020-11-08 14:43 ` 廖威雄
@ 2020-12-01 19:52 ` Kees Cook
1 sibling, 0 replies; 28+ messages in thread
From: Kees Cook @ 2020-12-01 19:52 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Anton Vorontsov, Colin Cross, Tony Luck, WeiXiong Liao,
linux-mtd, linux-kernel
On Fri, Oct 16, 2020 at 03:20:46PM +0200, Christoph Hellwig wrote:
> Stop poking into block layer internals and just open the block device
> file an use kernel_read and kernel_write on it. Note that this means
> the transformation from name_to_dev_t can't be used anymore, and proper
> block device file names must be used.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
> [...]
> +
> +#ifdef MODULE
> +static int __init pstore_blk_init(void)
> +{
> + return __pstore_blk_init(blkdev);
> +}
> late_initcall(pstore_blk_init);
>
> static void __exit pstore_blk_exit(void)
> {
> - if (!psblk_bdev)
> + if (!psblk_file)
> return;
> unregister_pstore_device(&pstore_blk_zone_ops);
> - blkdev_put(psblk_bdev, FMODE_READ | FMODE_WRITE | FMODE_EXCL);
> + fput(psblk_file);
> }
> module_exit(pstore_blk_exit);
> +#else /* MODULE */
> +/*
> + * During early boot the real root file system hasn't been mounted yet,
> + * and not device nodes are present yet. Use the same scheme to find
> + * the device that we use for mounting the root file system.
> + */
> +void __init pstore_blk_early_init(void)
> +{
> + const char devname[] = "/dev/pstore-blk";
> + dev_t dev = name_to_dev_t(blkdev);
> +
> + if (!dev)
> + return;
> + init_unlink(devname);
> + init_mknod(devname, S_IFBLK | 0600, new_encode_dev(dev));
> + __pstore_blk_init(devname);
> +}
> +#endif /* MODULE */
That the allowed naming conventions change based on _how_ pstore is
built seems very wrong to me.
While I do like the clean up to simplify the read/write activities, this
seems like totally the wrong approach here.
>
> /* get information of pstore/blk */
> int pstore_blk_get_config(struct pstore_blk_config *info)
> diff --git a/include/linux/pstore_blk.h b/include/linux/pstore_blk.h
> index 0abd412a6cb3e3..7c06b9d6740e2a 100644
> --- a/include/linux/pstore_blk.h
> +++ b/include/linux/pstore_blk.h
> @@ -39,4 +39,6 @@ struct pstore_blk_config {
> */
> int pstore_blk_get_config(struct pstore_blk_config *info);
>
> +void __init pstore_blk_early_init(void);
> +
> #endif
> diff --git a/init/main.c b/init/main.c
> index 1af84337cb18d5..058cce64f70390 100644
> --- a/init/main.c
> +++ b/init/main.c
> @@ -98,6 +98,7 @@
> #include <linux/mem_encrypt.h>
> #include <linux/kcsan.h>
> #include <linux/init_syscalls.h>
> +#include <linux/pstore_blk.h>
>
> #include <asm/io.h>
> #include <asm/bugs.h>
> @@ -1524,6 +1525,9 @@ static noinline void __init kernel_init_freeable(void)
> prepare_namespace();
> }
>
> + if (IS_BUILTIN(CONFIG_PSTORE_BLK))
> + pstore_blk_early_init();
> +
I hate this being a special-case in kernel_init. For ramoops, we use:
postcore_initcall(ramoops_init);
which is much better than open coding this here.
> /*
> * Ok, we have completed the initial bootup, and
> * we're essentially up and running. Get rid of the
> --
> 2.28.0
>
--
Kees Cook
^ permalink raw reply [flat|nested] 28+ messages in thread
* [PATCH 9/9] pstore/blk: don't depend on CONFIG_BLOCK
2020-10-16 13:20 simplify pstore-blk Christoph Hellwig
` (7 preceding siblings ...)
2020-10-16 13:20 ` [PATCH 8/9] pstore/blk: use the normal block device I/O path Christoph Hellwig
@ 2020-10-16 13:20 ` Christoph Hellwig
2020-12-01 19:53 ` Kees Cook
2020-10-16 22:54 ` simplify pstore-blk Kees Cook
2020-11-08 14:34 ` 廖威雄
10 siblings, 1 reply; 28+ messages in thread
From: Christoph Hellwig @ 2020-10-16 13:20 UTC (permalink / raw)
To: Kees Cook, Anton Vorontsov, Colin Cross, Tony Luck, WeiXiong Liao
Cc: linux-mtd, linux-kernel
pstore-blk contains of two different layers:
a) a tiny layer of sugar coating ontop of pstore-zone. This part has
no dependencies on the block layer, and can be used e.g. by mtd
b) an implementation of a default fallback pstore zone backend for
block devices
Add an ifdef for the latter so that pstore-blk itself does not have to
depend on CONFIG_BLOCK.
Signed-off-by: Christoph Hellwig <hch@lst.de>
---
fs/pstore/Kconfig | 2 +-
fs/pstore/blk.c | 2 ++
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/fs/pstore/Kconfig b/fs/pstore/Kconfig
index e16a49ebfe546d..6eadb538316e52 100644
--- a/fs/pstore/Kconfig
+++ b/fs/pstore/Kconfig
@@ -164,7 +164,7 @@ config PSTORE_ZONE
config PSTORE_BLK
tristate "Log panic/oops to a block device"
depends on PSTORE
- depends on BLOCK
+ depends on BLOCK || !BLOCK
select PSTORE_ZONE
default n
help
diff --git a/fs/pstore/blk.c b/fs/pstore/blk.c
index bd4eadfc9bd795..d3d25edb943cdd 100644
--- a/fs/pstore/blk.c
+++ b/fs/pstore/blk.c
@@ -182,6 +182,7 @@ void unregister_pstore_device(const struct pstore_zone_ops *ops)
}
EXPORT_SYMBOL_GPL(unregister_pstore_device);
+#ifdef CONFIG_BLOCK
static struct file *psblk_file;
static ssize_t psblk_generic_blk_read(char *buf, size_t bytes, loff_t pos)
@@ -269,6 +270,7 @@ void __init pstore_blk_early_init(void)
__pstore_blk_init(devname);
}
#endif /* MODULE */
+#endif /* CONFIG_BLOCK */
/* get information of pstore/blk */
int pstore_blk_get_config(struct pstore_blk_config *info)
--
2.28.0
^ permalink raw reply related [flat|nested] 28+ messages in thread
* Re: [PATCH 9/9] pstore/blk: don't depend on CONFIG_BLOCK
2020-10-16 13:20 ` [PATCH 9/9] pstore/blk: don't depend on CONFIG_BLOCK Christoph Hellwig
@ 2020-12-01 19:53 ` Kees Cook
0 siblings, 0 replies; 28+ messages in thread
From: Kees Cook @ 2020-12-01 19:53 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Anton Vorontsov, Colin Cross, Tony Luck, WeiXiong Liao,
linux-mtd, linux-kernel
On Fri, Oct 16, 2020 at 03:20:47PM +0200, Christoph Hellwig wrote:
> pstore-blk contains of two different layers:
>
> a) a tiny layer of sugar coating ontop of pstore-zone. This part has
> no dependencies on the block layer, and can be used e.g. by mtd
> b) an implementation of a default fallback pstore zone backend for
> block devices
>
> Add an ifdef for the latter so that pstore-blk itself does not have to
> depend on CONFIG_BLOCK.
>
> Signed-off-by: Christoph Hellwig <hch@lst.de>
AFAIU, this can't be done until the read/write changes from patch 8 are
adopted, so, for now, I'm not taking this either.
-Kees
> ---
> fs/pstore/Kconfig | 2 +-
> fs/pstore/blk.c | 2 ++
> 2 files changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/fs/pstore/Kconfig b/fs/pstore/Kconfig
> index e16a49ebfe546d..6eadb538316e52 100644
> --- a/fs/pstore/Kconfig
> +++ b/fs/pstore/Kconfig
> @@ -164,7 +164,7 @@ config PSTORE_ZONE
> config PSTORE_BLK
> tristate "Log panic/oops to a block device"
> depends on PSTORE
> - depends on BLOCK
> + depends on BLOCK || !BLOCK
> select PSTORE_ZONE
> default n
> help
> diff --git a/fs/pstore/blk.c b/fs/pstore/blk.c
> index bd4eadfc9bd795..d3d25edb943cdd 100644
> --- a/fs/pstore/blk.c
> +++ b/fs/pstore/blk.c
> @@ -182,6 +182,7 @@ void unregister_pstore_device(const struct pstore_zone_ops *ops)
> }
> EXPORT_SYMBOL_GPL(unregister_pstore_device);
>
> +#ifdef CONFIG_BLOCK
> static struct file *psblk_file;
>
> static ssize_t psblk_generic_blk_read(char *buf, size_t bytes, loff_t pos)
> @@ -269,6 +270,7 @@ void __init pstore_blk_early_init(void)
> __pstore_blk_init(devname);
> }
> #endif /* MODULE */
> +#endif /* CONFIG_BLOCK */
>
> /* get information of pstore/blk */
> int pstore_blk_get_config(struct pstore_blk_config *info)
> --
> 2.28.0
>
--
Kees Cook
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: simplify pstore-blk
2020-10-16 13:20 simplify pstore-blk Christoph Hellwig
` (8 preceding siblings ...)
2020-10-16 13:20 ` [PATCH 9/9] pstore/blk: don't depend on CONFIG_BLOCK Christoph Hellwig
@ 2020-10-16 22:54 ` Kees Cook
2020-11-23 14:53 ` Christoph Hellwig
2020-11-08 14:34 ` 廖威雄
10 siblings, 1 reply; 28+ messages in thread
From: Kees Cook @ 2020-10-16 22:54 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Anton Vorontsov, Colin Cross, Tony Luck, WeiXiong Liao,
linux-mtd, linux-kernel
On Fri, Oct 16, 2020 at 03:20:38PM +0200, Christoph Hellwig wrote:
> this series cleans up and massively simplifies the pstore-blk code,
> please take a look.
Cool! Thanks for doing this work. I have a few things I'd like to see
done differently, and while I'm not a huge fan of the general reduction
in utility, I can live with it as long as it doesn't make other things
worse. :) I'll get this reviewed with specific feedback soon, but I'm
about to be EOW. ;)
--
Kees Cook
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: simplify pstore-blk
2020-10-16 22:54 ` simplify pstore-blk Kees Cook
@ 2020-11-23 14:53 ` Christoph Hellwig
2020-11-24 22:53 ` Kees Cook
0 siblings, 1 reply; 28+ messages in thread
From: Christoph Hellwig @ 2020-11-23 14:53 UTC (permalink / raw)
To: Kees Cook
Cc: Christoph Hellwig, Anton Vorontsov, Colin Cross, Tony Luck,
WeiXiong Liao, linux-mtd, linux-kernel
On Fri, Oct 16, 2020 at 03:54:25PM -0700, Kees Cook wrote:
> On Fri, Oct 16, 2020 at 03:20:38PM +0200, Christoph Hellwig wrote:
> > this series cleans up and massively simplifies the pstore-blk code,
> > please take a look.
>
> Cool! Thanks for doing this work. I have a few things I'd like to see
> done differently, and while I'm not a huge fan of the general reduction
> in utility, I can live with it as long as it doesn't make other things
> worse. :) I'll get this reviewed with specific feedback soon, but I'm
> about to be EOW. ;)
Any progress on this in the last five weeks?
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: simplify pstore-blk
2020-11-23 14:53 ` Christoph Hellwig
@ 2020-11-24 22:53 ` Kees Cook
0 siblings, 0 replies; 28+ messages in thread
From: Kees Cook @ 2020-11-24 22:53 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Anton Vorontsov, Colin Cross, Tony Luck, WeiXiong Liao,
linux-mtd, linux-kernel
On Mon, Nov 23, 2020 at 03:53:31PM +0100, Christoph Hellwig wrote:
> On Fri, Oct 16, 2020 at 03:54:25PM -0700, Kees Cook wrote:
> > On Fri, Oct 16, 2020 at 03:20:38PM +0200, Christoph Hellwig wrote:
> > > this series cleans up and massively simplifies the pstore-blk code,
> > > please take a look.
> >
> > Cool! Thanks for doing this work. I have a few things I'd like to see
> > done differently, and while I'm not a huge fan of the general reduction
> > in utility, I can live with it as long as it doesn't make other things
> > worse. :) I'll get this reviewed with specific feedback soon, but I'm
> > about to be EOW. ;)
>
> Any progress on this in the last five weeks?
Hi! Sorry, no, I keep getting distracted by other stuff. I'll try to
make time for it again next week (I'm soon AFK for Turkey Day).
--
Kees Cook
^ permalink raw reply [flat|nested] 28+ messages in thread
* Re: simplify pstore-blk
2020-10-16 13:20 simplify pstore-blk Christoph Hellwig
` (9 preceding siblings ...)
2020-10-16 22:54 ` simplify pstore-blk Kees Cook
@ 2020-11-08 14:34 ` 廖威雄
10 siblings, 0 replies; 28+ messages in thread
From: 廖威雄 @ 2020-11-08 14:34 UTC (permalink / raw)
To: Christoph Hellwig
Cc: Kees Cook, Anton Vorontsov, Colin Cross, Tony Luck,
WeiXiong Liao, linux-mtd, linux-kernel
hi Christoph,
On Fri, Oct 16, 2020 at 9:27 PM Christoph Hellwig <hch@lst.de> wrote:
>
> Hi all,
>
> this series cleans up and massively simplifies the pstore-blk code,
> please take a look.
Thanks for your code. I am going to redesign pstore/blk referred to
your idea.
I want to split pstore/blk into two parts, pstore/dev and pstore/blk.
pstore/dev is a common layer that handles all configurations
for users. All devices including block devices, ram devices and
mtd devices register to pstore/dev with operations and some flags.
Besides that, we can delete the codes for the block device to
register a panic_wirte() interface since there is no block device
needed. But it should be easy to implement registering if necessary.
^ permalink raw reply [flat|nested] 28+ messages in thread