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
