From: Kees Cook <keescook@chromium.org>
To: Christoph Hellwig <hch@lst.de>
Cc: Tony Luck <tony.luck@intel.com>,
Anton Vorontsov <anton@enomsg.org>,
linux-kernel@vger.kernel.org,
WeiXiong Liao <liaoweixiong@allwinnertech.com>,
linux-mtd@lists.infradead.org, Colin Cross <ccross@android.com>
Subject: Re: [PATCH 7/9] pstore/blk: remove struct pstore_device_info
Date: Tue, 1 Dec 2020 11:44:35 -0800 [thread overview]
Message-ID: <202012011143.43273D716@keescook> (raw)
In-Reply-To: <20201016132047.3068029-8-hch@lst.de>
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
______________________________________________________
Linux MTD discussion mailing list
http://lists.infradead.org/mailman/listinfo/linux-mtd/
next prev parent reply other threads:[~2020-12-01 19:45 UTC|newest]
Thread overview: 28+ messages / expand[flat|nested] mbox.gz Atom feed top
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-11-08 14:05 ` 廖威雄
2020-12-01 19:08 ` Kees Cook
2020-12-01 21:11 ` Kees Cook
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
2020-10-16 13:20 ` [PATCH 3/9] pstore/blk: remove {un,}register_pstore_blk 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
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
2020-12-01 19:40 ` Kees Cook
2020-10-16 13:20 ` [PATCH 6/9] pstore/zone: split struct pstore_zone_info 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
2020-12-01 19:44 ` Kees Cook [this message]
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
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
2020-10-16 22:54 ` simplify pstore-blk Kees Cook
2020-11-23 14:53 ` Christoph Hellwig
2020-11-24 22:53 ` Kees Cook
2020-11-08 14:34 ` 廖威雄
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=202012011143.43273D716@keescook \
--to=keescook@chromium.org \
--cc=anton@enomsg.org \
--cc=ccross@android.com \
--cc=hch@lst.de \
--cc=liaoweixiong@allwinnertech.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mtd@lists.infradead.org \
--cc=tony.luck@intel.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).