Date: Thu, 20 Jun 2024 13:17:09 +0000
From: Peng Fan <peng.fan@....com>
To: Cristian Marussi <cristian.marussi@....com>, "Peng Fan (OSS)"
	<peng.fan@....nxp.com>
CC: Rob Herring <robh@...nel.org>, Krzysztof Kozlowski
	<krzysztof.kozlowski+dt@...aro.org>, Conor Dooley <conor+dt@...nel.org>,
	Sudeep Holla <sudeep.holla@....com>, Shawn Guo <shawnguo@...nel.org>, Sascha
 Hauer <s.hauer@...gutronix.de>, Pengutronix Kernel Team
	<kernel@...gutronix.de>, Fabio Estevam <festevam@...il.com>, dl-linux-imx
	<linux-imx@....com>, "devicetree@...r.kernel.org"
	<devicetree@...r.kernel.org>, "linux-kernel@...r.kernel.org"
	<linux-kernel@...r.kernel.org>, "linux-arm-kernel@...ts.infradead.org"
	<linux-arm-kernel@...ts.infradead.org>
Subject: RE: [PATCH 4/5] firmware: imx: support BBM module

Hi Cristian,

> Subject: Re: [PATCH 4/5] firmware: imx: support BBM module
> 
....

> > +msecs_to_jiffies(DEBOUNCE_TIME));
> > +
> > +	/*
> > +	 * Directly report key event after resume to make no key press
> > +	 * event is missed.
> > +	 */
> > +	if (bbnsm->suspended) {
> 
> So this bbnsm->suspended is checked here from inside the SCMI
> notifier and it is set by a couple of pm_ops you provide down below
> which are called by the core PM subsys, so is it not high likely that you
> could have issues with the order of such reads/writes ?
> 
> Would it be worth to add a READ_ONCE here and WRITE_ONCE in the
> pm_ops...or I am overthinking ?

Just checked other input drivers, only two use READ_ONCE.
It might be good to avoid potential issues with READ/WRITE_ONCE.

Other comments will be addressed. BTW,
I will split rtc and pwr key into two drivers and put
them in input and rtc directory.

Thanks,
Peng

> 
> > +		bbnsm->keystate = 1;
> > +		input_event(input, EV_KEY, bbnsm->keycode, 1);
> > +		input_sync(input);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static void scmi_imx_bbm_pwrkey_act(void *pdata) {
> > +	struct scmi_imx_bbm *bbnsm = pdata;
> > +
> > +	cancel_delayed_work_sync(&bbnsm->check_work);
> > +}
> > +
> > +static int scmi_imx_bbm_notifier(struct notifier_block *nb, unsigned
> > +long event, void *data) {
> > +	struct scmi_imx_bbm *bbnsm = container_of(nb, struct
> scmi_imx_bbm, nb);
> > +	struct scmi_imx_bbm_notif_report *r = data;
> > +
> > +	if (r->is_rtc)
> > +		rtc_update_irq(bbnsm->rtc_dev, 1, RTC_AF |
> RTC_IRQF);
> > +	if (r->is_button) {
> > +		pr_debug("BBM Button Power key pressed\n");
> > +		scmi_imx_bbm_pwrkey_event(bbnsm);
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int scmi_imx_bbm_pwrkey_init(struct scmi_device *sdev) {
> > +	const struct scmi_handle *handle = sdev->handle;
> > +	struct device *dev = &sdev->dev;
> > +	struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev);
> > +	struct input_dev *input;
> > +	int ret;
> > +
> > +	if (device_property_read_u32(dev, "linux,code", &bbnsm-
> >keycode)) {
> > +		bbnsm->keycode = KEY_POWER;
> > +		dev_warn(dev, "key code is not specified, using
> default KEY_POWER\n");
> > +	}
> > +
> > +	INIT_DELAYED_WORK(&bbnsm->check_work,
> > +scmi_imx_bbm_pwrkey_check_for_events);
> > +
> > +	input = devm_input_allocate_device(dev);
> > +	if (!input) {
> > +		dev_err(dev, "failed to allocate the input device for
> SCMI IMX BBM\n");
> > +		return -ENOMEM;
> > +	}
> > +
> > +	input->name = dev_name(dev);
> > +	input->phys = "bbnsm-pwrkey/input0";
> > +	input->id.bustype = BUS_HOST;
> > +
> > +	input_set_capability(input, EV_KEY, bbnsm->keycode);
> > +
> > +	ret = devm_add_action_or_reset(dev,
> scmi_imx_bbm_pwrkey_act, bbnsm);
> > +	if (ret) {
> > +		dev_err(dev, "failed to register remove action\n");
> > +		return ret;
> > +	}
> > +
> > +	bbnsm->input = input;
> > +
> > +	ret = handle->notify_ops->devm_event_notifier_register(sdev,
> SCMI_PROTOCOL_IMX_BBM,
> > +
> SCMI_EVENT_IMX_BBM_BUTTON,
> > +							       NULL,
> &bbnsm->nb);
> 
> So you are registering for another SCMI event but you want to use the
> same callback and notifier_bock to handle different events, BUT
> internally the SCMI core creates a DISTINCT kernel regular notification
> chain for each event and each resource (or one for ALL resources of an
> event) against which a
> devm_event_notifier_register() has been called AND so, being a
> notification_chain the notifier_blocks that you provide MUST be
> distinct, because the notification chain is indeed a simply-linked list
> and so when you register bbnsm->nb the second time you will indeed
> add the nb to another list at the same....
> 
> ...thing which I suppose could work in your case since you have only
> nb/callback per event BUT as soon as you (or someone else) will try to
> register another nb for these same events the 2 notification chains will
> start melting together....
> 
> ...and it will be a hell to debug...
> 
> so IOW...even if it works now for you, please use 2 distinct nb_pwr.
> nb_rtc notifier blocks with 2 distinct callbacks (to be able to use
> container_of in
> them) to register to 2 distinct events...you can register for multiple
> sources using only one nb BUT you cannot register for multiple events
> using the same nb/callback as of now.
> 
> With this internal design the queues and the worker threads
> dispatching these notifs are dedicated to a single event and possible to
> a single event/resource...
> ...no event ever queues behind any other...
> 
> This probably would need better clarification in the SCMI docs, my bad,
> and maybe a new option to register for ANY event the same nb (like
> you can do with src_id if you dont specify one), IF you are fine with the
> possibility that your events notification will be serialized in a single
> queue.
> 
> > +
> > +	if (ret)
> > +		dev_err(dev, "Failed to register BBM Button
> Events %d:", ret);
> > +
> 
> So why not failing if you could NOT register the notifications ?
> 
> > +	ret = input_register_device(input);
> > +	if (ret) {
> > +		dev_err(dev, "failed to register input device\n");
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int scmi_imx_bbm_rtc_init(struct scmi_device *sdev) {
> > +	const struct scmi_handle *handle = sdev->handle;
> > +	struct device *dev = &sdev->dev;
> > +	struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev);
> > +	int ret;
> > +
> > +	bbnsm->rtc_dev = devm_rtc_allocate_device(dev);
> > +	if (IS_ERR(bbnsm->rtc_dev))
> > +		return PTR_ERR(bbnsm->rtc_dev);
> > +
> > +	bbnsm->rtc_dev->ops = &smci_imx_bbm_rtc_ops;
> > +	bbnsm->rtc_dev->range_min = 0;
> > +	bbnsm->rtc_dev->range_max = U32_MAX;
> > +
> > +	ret = devm_rtc_register_device(bbnsm->rtc_dev);
> > +	if (ret)
> > +		return ret;
> > +
> > +	bbnsm->nb.notifier_call = &scmi_imx_bbm_notifier;
> > +	return handle->notify_ops->devm_event_notifier_register(sdev,
> SCMI_PROTOCOL_IMX_BBM,
> > +
> 	SCMI_EVENT_IMX_BBM_RTC,
> > +
> 	NULL, &bbnsm->nb);
> 
> As said, this will get mixed up when pwrkey_init tries to register again
> the same nb for a different event...
> 
> > +}
> > +
> > +static int scmi_imx_bbm_probe(struct scmi_device *sdev) {
> > +	const struct scmi_handle *handle = sdev->handle;
> > +	struct device *dev = &sdev->dev;
> > +	struct scmi_protocol_handle *ph;
> > +	struct scmi_imx_bbm *bbnsm;
> > +	int ret;
> > +
> > +	if (!handle)
> > +		return -ENODEV;
> > +
> > +	bbnsm = devm_kzalloc(dev, sizeof(struct scmi_imx_bbm),
> GFP_KERNEL);
> 
> sizeof(*bbnsm)
> 
> > +	if (!bbnsm)
> > +		return -ENOMEM;
> > +
> > +	bbnsm->ops = handle->devm_protocol_get(sdev,
> SCMI_PROTOCOL_IMX_BBM,
> > +&ph);
> 
> proto ops can be global really..since are always the same pointer even
> if this is probed mutiple times... this could be
> 
> 	bbnsm_ops = handle->devm_protocol_get(sdev,
> SCMI_PROTOCOL_IMX_BBM, &bbnsm->ph);
> 
> with bbnsm_ops static global to this file
> 
> > +	if (IS_ERR(bbnsm->ops))
> > +		return PTR_ERR(bbnsm->ops);
> > +
> > +	bbnsm->ph = ph;
> > +
> > +	device_init_wakeup(dev, true);
> 
> Not fully familiar with this, but it seems to me that when this is issued
> some wakeup related sysfs entries are created too...so I suppose you
> want to disable this back on failure to have those entries removed.
> 
> or maybe just move this right before the final return 0....but I am not
> sure if you want to have it called BEFORE the pwrkey notifier if
> registered or AFTER is fine too...potentially loosing some wakeup,
> though.
> 
> > +
> > +	dev_set_drvdata(dev, bbnsm);
> > +
> > +	ret = scmi_imx_bbm_rtc_init(sdev);
> > +	if (ret) {
> > +		dev_err(dev, "rtc init failed: %d\n", ret);
> 
> like ??
> 		device_init_wakeup(dev, false);
> 
> > +		return ret;
> > +	}
> > +
> > +	ret = scmi_imx_bbm_pwrkey_init(sdev);
> > +	if (ret) {
> > +		dev_err(dev, "pwr init failed: %d\n", ret);
> and...
> 		device_init_wakeup(dev, false);
> 
> > +		return ret;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused scmi_imx_bbm_suspend(struct device
> *dev) {
> > +	struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev);
> > +
> > +	bbnsm->suspended = true;
> > +
> > +	return 0;
> > +}
> > +
> > +static int __maybe_unused scmi_imx_bbm_resume(struct device
> *dev) {
> > +	struct scmi_imx_bbm *bbnsm = dev_get_drvdata(dev);
> > +
> > +	bbnsm->suspended = false;
> > +
> > +	return 0;
> > +}
> > +
> > +static SIMPLE_DEV_PM_OPS(scmi_imx_bbm_pm_ops,
> scmi_imx_bbm_suspend,
> > +scmi_imx_bbm_resume);
> > +
> > +static const struct scmi_device_id scmi_id_table[] = {
> > +	{ SCMI_PROTOCOL_IMX_BBM, "imx-bbm" },
> > +	{ },
> > +};
> > +MODULE_DEVICE_TABLE(scmi, scmi_id_table);
> > +
> > +static struct scmi_driver scmi_imx_bbm_driver = {
> > +	.driver = {
> > +		.pm = &scmi_imx_bbm_pm_ops,
> > +	},
> > +	.name = "scmi-imx-bbm",
> > +	.probe = scmi_imx_bbm_probe,
> > +	.id_table = scmi_id_table,
> > +};
> > +module_scmi_driver(scmi_imx_bbm_driver);
> > +
> 
> Thanks,
> Cristian

Powered by blists - more mailing lists