From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751263AbdCYODi (ORCPT ); Sat, 25 Mar 2017 10:03:38 -0400 Received: from mx1.redhat.com ([209.132.183.28]:49752 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751192AbdCYODg (ORCPT ); Sat, 25 Mar 2017 10:03:36 -0400 DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com A189764D81 Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com Authentication-Results: ext-mx09.extmail.prod.ext.phx2.redhat.com; spf=pass smtp.mailfrom=hdegoede@redhat.com DKIM-Filter: OpenDKIM Filter v2.11.0 mx1.redhat.com A189764D81 Subject: Re: [PATCH v2] extcon: Allow registering a single notifier for all cables on an extcon_dev To: Chanwoo Choi , MyungJoo Ham References: <20170319182334.6456-1-hdegoede@redhat.com> <58CF288A.9090507@samsung.com> Cc: linux-kernel@vger.kernel.org From: Hans de Goede Message-ID: <0ea05e7a-a86a-b770-f113-c85d0e3fc11e@redhat.com> Date: Sat, 25 Mar 2017 15:03:33 +0100 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.8.0 MIME-Version: 1.0 In-Reply-To: Content-Type: text/plain; charset=utf-8; format=flowed Content-Transfer-Encoding: 8bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.38]); Sat, 25 Mar 2017 14:03:36 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi, On 20-03-17 10:48, Hans de Goede wrote: > Hi, > > On 20-03-17 01:55, Chanwoo Choi wrote: >> Hi, >> >> On 2017년 03월 20일 03:23, Hans de Goede wrote: >>> In some cases a driver may want to monitor multiple cables on a single >>> extcon. For example a charger driver will typically want to monitor all >>> of EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP to configure >>> the max. current it can sink while charging. >>> >>> Due to the signature of the notifier_call function + how extcon passes >>> state and the extcon_dev as parameters this requires using one >>> notifier_block + one notifier_call function per cable, otherwise the >>> notifier_call function cannot get to its driver's data (using container_of >>> requires it to know which notifier block its first parameter is). >>> >>> For a driver wanting to monitor the above 3 cables that would result >>> in something like this: >>> >>> static const unsigned int vbus_cable_ids[] = { >>> EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP }; >>> >>> struct driver_data { >>> struct notifier_block vbus_nb[ARRAY_SIZE(vbus_cable_ids)]; >>> } >>> >>> /* >>> * We need 3 copies of this, because there is no way to find out for which >>> * cable id we are being called from the passed in arguments; and we must >>> * have a separate nb for each extcon_register_notifier call. >>> */ >>> static int vbus_cable0_evt(struct notifier_block *nb, unsigned long e, void *p) >>> { >>> struct driver_data *data = >>> container_of(nb, struct driver_data, vbus_nb[0]); >>> ... >>> } >>> >>> static int vbus_cable1_evt(struct notifier_block *nb, unsigned long e, void *p) >>> { >>> struct driver_data *data = >>> container_of(nb, struct driver_data, vbus_nb[1]); >>> ... >>> } >>> >>> static int vbus_cable2_evt(struct notifier_block *nb, unsigned long e, void *p) >>> { >>> struct driver_data *data = >>> container_of(nb, struct driver_data, vbus_nb[2]); >>> ... >>> } >>> >>> int probe(...) >>> { >>> /* Register for vbus notification */ >>> data->vbus_nb[0].notifier_call = vbus_cable0_evt; >>> data->vbus_nb[1].notifier_call = vbus_cable1_evt; >>> data->vbus_nb[2].notifier_call = vbus_cable2_evt; >>> for (i = 0; i < ARRAY_SIZE(vbus_cable_ids); i++) { >>> ret = devm_extcon_register_notifier(dev, data->vbus_extcon, >>> vbus_cable_ids[i], &data->vbus_nb[i]); >>> if (ret) >>> ... >>> } >>> } >> >> You can get the notification for multiple external connector >> by using the only one notifier_block as following: >> >> static const unsigned int vbus_cable_ids[] = { >> EXTCON_CHG_USB_SDP, EXTCON_CHG_USB_CDP, EXTCON_CHG_USB_DCP }; >> >> struct driver_data { >> struct notifier_block vbus_nb; >> } >> >> static int vbus_cable_evt(struct notifier_block *nb, unsigned long e, void *p) >> { >> struct driver_data *data = >> container_of(nb, struct driver_data, vbus_nb); >> int sdp_state, cdp_state, dcp_state; >> >> sdp_state = extcon_get_state(data->vbus_extcon, EXTCON_CHG_USB_SDP); >> cdp_state = extcon_get_state(data->vbus_extcon, EXTCON_CHG_USB_CDP); >> dcp_state = extcon_get_state(data->vbus_extcon, EXTCON_CHG_USB_DCP); >> >> ... >> } >> >> int probe(...) >> { >> /* Register for vbus notification */ >> data->vbus_nb.notifier_call = vbus_cable_evt; >> for (i = 0; i < ARRAY_SIZE(vbus_cable_ids); i++) { >> ret = devm_extcon_register_notifier(dev, data->vbus_extcon, >> vbus_cable_ids[i], &data->vbus_nb); >> if (ret) >> ... >> } >> } > > No that will not work, you cannot add the same notifier_block to 3 > different notifier_heads, the list_head can only be part of one list! > > Also see: > > https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/power/supply?id=577b1f06e22057e9cdc14b1ee5bd25435c71ff0f > > Which actually fixes an issue I encountered exactly because of doing > the above. > > So we really need something like my patch to make this simpler > and to remove the ugliness we currently have in axp288_charger.c > because of this problem (and are about to have in more drivers > I'm upstreaming). Also note that my patch is quite simple, the > amount of lines it adds is way less then the amount of lines > it will allow to remove in a single driver! Ping ? As explained this is a real problem and we really need a proper solution for this. I'm about to upstream 2 more drivers which need to monitor an antire extcon rather then a single cable on it, and I do not want to have to add hacks like this: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/drivers/power/supply?id=577b1f06e22057e9cdc14b1ee5bd25435c71ff0f To both drivers. My patch is IMHO a nice and clean solution for this, can you please merge this or provide an alternative solution ? Regards, Hans > > Regards, > > Hans > > >>> >>> And then in the event handling the driver often checks the state of >>> all cables explicitly using extcon_get_state, rather then using the >>> event argument to the notifier_call. >>> >>> This commit makes extcon_[un]register_notifier accept -1 as cable-id, >>> which will cause the notifier to get called for changes on any cable >>> on the extcon_dev. Compared to the above example code this allows much >>> simpler code in drivers which want to monitor multiple cable types. >>> >>> Signed-off-by: Hans de Goede >>> --- >>> Changes in v2: >>> -Fix false-positive "warning: 'idx' may be used uninitialized" warning >>> seen with some compiler versions >>> --- >>> drivers/extcon/extcon.c | 35 ++++++++++++++++++++++++++--------- >>> drivers/extcon/extcon.h | 1 + >>> 2 files changed, 27 insertions(+), 9 deletions(-) >>> >>> diff --git a/drivers/extcon/extcon.c b/drivers/extcon/extcon.c >>> index 09ac5e7..2a042ee 100644 >>> --- a/drivers/extcon/extcon.c >>> +++ b/drivers/extcon/extcon.c >>> @@ -449,6 +449,7 @@ int extcon_sync(struct extcon_dev *edev, unsigned int id) >>> >>> state = !!(edev->state & BIT(index)); >>> raw_notifier_call_chain(&edev->nh[index], state, edev); >>> + raw_notifier_call_chain(&edev->nh_all, 0, edev); >>> >>> /* This could be in interrupt handler */ >>> prop_buf = (char *)get_zeroed_page(GFP_ATOMIC); >>> @@ -900,6 +901,8 @@ EXPORT_SYMBOL_GPL(extcon_get_extcon_dev); >>> * any attach status changes from the extcon. >>> * @edev: the extcon device that has the external connecotr. >>> * @id: the unique id of each external connector in extcon enumeration. >>> + * or -1 to get notififications for all cables on edev, in this >>> + * case no state info will get passed to the notifier_call. >>> * @nb: a notifier block to be registered. >>> * >>> * Note that the second parameter given to the callback of nb (val) is >>> @@ -915,12 +918,18 @@ int extcon_register_notifier(struct extcon_dev *edev, unsigned int id, >>> if (!edev || !nb) >>> return -EINVAL; >>> >>> - idx = find_cable_index_by_id(edev, id); >>> - if (idx < 0) >>> - return idx; >>> + if ((int)id != -1) { >>> + idx = find_cable_index_by_id(edev, id); >>> + if (idx < 0) >>> + return idx; >>> + } >>> >>> spin_lock_irqsave(&edev->lock, flags); >>> - ret = raw_notifier_chain_register(&edev->nh[idx], nb); >>> + if ((int)id != -1) >>> + ret = raw_notifier_chain_register(&edev->nh[idx], nb); >>> + else >>> + ret = raw_notifier_chain_register(&edev->nh_all, nb); >>> + >>> spin_unlock_irqrestore(&edev->lock, flags); >>> >>> return ret; >>> @@ -937,17 +946,23 @@ int extcon_unregister_notifier(struct extcon_dev *edev, unsigned int id, >>> struct notifier_block *nb) >>> { >>> unsigned long flags; >>> - int ret, idx; >>> + int ret, idx = 0; >>> >>> if (!edev || !nb) >>> return -EINVAL; >>> >>> - idx = find_cable_index_by_id(edev, id); >>> - if (idx < 0) >>> - return idx; >>> + if ((int)id != -1) { >>> + idx = find_cable_index_by_id(edev, id); >>> + if (idx < 0) >>> + return idx; >>> + } >>> >>> spin_lock_irqsave(&edev->lock, flags); >>> - ret = raw_notifier_chain_unregister(&edev->nh[idx], nb); >>> + if ((int)id != -1) >>> + ret = raw_notifier_chain_unregister(&edev->nh[idx], nb); >>> + else >>> + ret = raw_notifier_chain_unregister(&edev->nh_all, nb); >>> + >>> spin_unlock_irqrestore(&edev->lock, flags); >>> >>> return ret; >>> @@ -1212,6 +1227,8 @@ int extcon_dev_register(struct extcon_dev *edev) >>> for (index = 0; index < edev->max_supported; index++) >>> RAW_INIT_NOTIFIER_HEAD(&edev->nh[index]); >>> >>> + RAW_INIT_NOTIFIER_HEAD(&edev->nh_all); >>> + >>> dev_set_drvdata(&edev->dev, edev); >>> edev->state = 0; >>> >>> diff --git a/drivers/extcon/extcon.h b/drivers/extcon/extcon.h >>> index 993ddcc..2e6c09d 100644 >>> --- a/drivers/extcon/extcon.h >>> +++ b/drivers/extcon/extcon.h >>> @@ -44,6 +44,7 @@ struct extcon_dev { >>> /* Internal data. Please do not set. */ >>> struct device dev; >>> struct raw_notifier_head *nh; >>> + struct raw_notifier_head nh_all; >>> struct list_head entry; >>> int max_supported; >>> spinlock_t lock; /* could be called by irq handler */ >>> >> >>