From: Ulf Hansson <ulf.hansson@linaro.org>
To: Bhaskara Budiredla <bbudiredla@marvell.com>
Cc: Kees Cook <keescook@chromium.org>,
Colin Cross <ccross@android.com>, Tony Luck <tony.luck@intel.com>,
Sunil Kovvuri Goutham <sgoutham@marvell.com>,
"linux-mmc@vger.kernel.org" <linux-mmc@vger.kernel.org>,
Linux Kernel Mailing List <linux-kernel@vger.kernel.org>,
Christoph Hellwig <hch@lst.de>
Subject: Re: [EXT] Re: [PATCH v1 1/2] mmc: Support kmsg dumper based on pstore/blk
Date: Tue, 24 Nov 2020 15:40:21 +0100 [thread overview]
Message-ID: <CAPDyKFqZomrJDgjKWvxeOornENnZFXSX+NWTWEerNHf=zf1L8g@mail.gmail.com> (raw)
In-Reply-To: <CY4PR1801MB2070549B13D3ADD324F4E8EBDEFB0@CY4PR1801MB2070.namprd18.prod.outlook.com>
+ Christoph
On Tue, 24 Nov 2020 at 05:09, Bhaskara Budiredla <bbudiredla@marvell.com> wrote:
>
>
>
> >-----Original Message-----
> >From: Ulf Hansson <ulf.hansson@linaro.org>
> >Sent: Monday, November 23, 2020 5:49 PM
> >To: Bhaskara Budiredla <bbudiredla@marvell.com>
> >Cc: Kees Cook <keescook@chromium.org>; Colin Cross
> ><ccross@android.com>; Tony Luck <tony.luck@intel.com>; Sunil Kovvuri
> >Goutham <sgoutham@marvell.com>; linux-mmc@vger.kernel.org; Linux
> >Kernel Mailing List <linux-kernel@vger.kernel.org>
> >Subject: Re: [EXT] Re: [PATCH v1 1/2] mmc: Support kmsg dumper based on
> >pstore/blk
> >
> >[...]
> >
> >> >
> >> >As I said above, I would like to avoid host specific deployments from
> >> >being needed. Is there a way we can avoid this?
> >> >
> >>
> >> I don't see an alternative.
> >
> >Well, if not, can you please explain why?
> >
>
> The solution has to be polling based as panic write runs with interrupts disabled.
Right, that's a good reason. :-) Please clarify that in the commit
message and add some information around the definition of the new host
ops callbacks should be used.
> I am not sure if there is a way to write a polling function that works of all kinds
> of host/dma drivers. That’s the reason I have provided hooks to define host
> specific deployments. If you have better ideas, please help.
No, at this point I don't, sorry. I will think more about it.
>
> >[...]
> >
> >> >> +
> >> >> +void mmcpstore_card_set(struct mmc_card *card, const char
> >> >> +*disk_name) {
> >> >> + struct mmcpstore_context *cxt = &oops_cxt;
> >> >> + struct pstore_blk_config *conf = &cxt->conf;
> >> >> + struct pstore_device_info *dev = &cxt->dev;
> >> >> + struct block_device *bdev;
> >> >> + struct mmc_command *stop;
> >> >> + struct mmc_command *cmd;
> >> >> + struct mmc_request *mrq;
> >> >> + struct mmc_data *data;
> >> >> + int ret;
> >> >> +
> >> >> + if (!conf->device[0])
> >> >> + return;
> >> >> +
> >> >> + /* Multiple backend devices not allowed */
> >> >> + if (cxt->dev_name[0])
> >> >> + return;
> >> >> +
> >> >> + bdev = mmcpstore_open_backend(conf->device);
> >> >> + if (IS_ERR(bdev)) {
> >> >> + pr_err("%s failed to open with %ld\n",
> >> >> + conf->device, PTR_ERR(bdev));
> >> >> + return;
> >> >> + }
> >> >> +
> >> >> + bdevname(bdev, cxt->dev_name);
> >> >> + cxt->partno = bdev->bd_part->partno;
> >> >> + mmcpstore_close_backend(bdev);
> >> >> +
> >> >> + if (strncmp(cxt->dev_name, disk_name, strlen(disk_name)))
> >> >> + return;
> >> >> +
> >> >> + cxt->start_sect = mmc_blk_get_part(card, cxt->partno, &cxt->size);
> >> >> + if (!cxt->start_sect) {
> >> >> + pr_err("Non-existent partition %d selected\n", cxt->partno);
> >> >> + return;
> >> >> + }
> >> >> +
> >> >> + /* Check for host mmc panic write polling function definitions */
> >> >> + if (!card->host->ops->req_cleanup_pending ||
> >> >> + !card->host->ops->req_completion_poll)
> >> >> + return;
> >> >> +
> >> >> + cxt->card = card;
> >> >> +
> >> >> + cxt->sub = kmalloc(conf->kmsg_size, GFP_KERNEL);
> >> >> + if (!cxt->sub)
> >> >> + goto out;
> >> >> +
> >> >> + mrq = kzalloc(sizeof(struct mmc_request), GFP_KERNEL);
> >> >> + if (!mrq)
> >> >> + goto free_sub;
> >> >> +
> >> >> + cmd = kzalloc(sizeof(struct mmc_command), GFP_KERNEL);
> >> >> + if (!cmd)
> >> >> + goto free_mrq;
> >> >> +
> >> >> + stop = kzalloc(sizeof(struct mmc_command), GFP_KERNEL);
> >> >> + if (!stop)
> >> >> + goto free_cmd;
> >> >> +
> >> >> + data = kzalloc(sizeof(struct mmc_data), GFP_KERNEL);
> >> >> + if (!data)
> >> >> + goto free_stop;
> >> >> +
> >> >> + mrq->cmd = cmd;
> >> >> + mrq->data = data;
> >> >> + mrq->stop = stop;
> >> >> + cxt->mrq = mrq;
> >> >> +
> >> >> + dev->total_size = cxt->size;
> >> >> + dev->flags = PSTORE_FLAGS_DMESG;
> >> >> + dev->read = mmcpstore_read;
> >> >> + dev->write = mmcpstore_write;
> >> >> + dev->erase = NULL;
> >> >> + dev->panic_write = mmcpstore_panic_write;
> >> >> +
> >> >> + ret = register_pstore_device(&cxt->dev);
> >> >
> >> >By looking at all of the code above, lots are duplicated from the mmc
> >> >block device implementation. Isn't there a way to make the pstore
> >> >block device to push a request through the regular blk-mq path instead?
> >> >
> >> The regular path has pre, post processing’s and locking semantics that
> >> are not suitable for panic write scenario. Further, the locking
> >> mechanisms are implemented in host drivers. This is preferred to
> >> quickly complete the write before the kernel dies.
> >
> >I am sorry, but this doesn't make sense to me.
>
> It seems there was some confusion. My comments were specific to
> mmcpstore_panic_write() as it runs with interrupts disabled.
> mmcpstore_read()/mmcpstore_write() indeed go through regular
> blk-mq path.
>
> >
> >When it comes to complete the data write, the regular block I/O path is
> >supposed to be optimized. If there is a problem with this path, then we should
> >fix it, rather than adding a new path along the side (unless there are very good
> >reasons not to).
> >
> >>
> >> >That said, I wonder why you don't call register_pstore_blk(), as I
> >> >thought that was the interface to be used for regular block devices, no?
> >> >
> >> register_pstore_blk() is for arbitrary block devices for which best effort is not
> >defined.
> >
> >Exactly why isn't "best effort" good enough for mmc?
> >
>
> register_pstore_blk() definitely does the work. If you prefer to take that route,
> it should be fine.
It looks like Christoph is planning for some rewrite of the pstore
code, so let's see what that means in regards to this.
[...]
Kind regards
Uffe
next prev parent reply other threads:[~2020-11-24 14:41 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-11-12 6:24 [PATCH v1 0/2] mmc: support crash logging to MMC block devices Bhaskara Budiredla
2020-11-12 6:24 ` [PATCH v1 1/2] mmc: Support kmsg dumper based on pstore/blk Bhaskara Budiredla
2020-11-13 13:58 ` kernel test robot
2020-11-20 13:20 ` Ulf Hansson
2020-11-23 5:41 ` [EXT] " Bhaskara Budiredla
2020-11-23 12:18 ` Ulf Hansson
2020-11-24 4:09 ` Bhaskara Budiredla
2020-11-24 14:40 ` Ulf Hansson [this message]
2020-11-24 16:18 ` Christoph Hellwig
2020-11-26 5:00 ` Bhaskara Budiredla
2020-11-12 6:24 ` [PATCH v1 2/2] mmc:cavium: Add MMC polling method to support kmsg panic/oops write Bhaskara Budiredla
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='CAPDyKFqZomrJDgjKWvxeOornENnZFXSX+NWTWEerNHf=zf1L8g@mail.gmail.com' \
--to=ulf.hansson@linaro.org \
--cc=bbudiredla@marvell.com \
--cc=ccross@android.com \
--cc=hch@lst.de \
--cc=keescook@chromium.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mmc@vger.kernel.org \
--cc=sgoutham@marvell.com \
--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).