| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Date: Wed, 13 Apr 2022 13:38:48 +0300
From: Roger Quadros <rogerq@...nel.org>
To: Aswath Govindraju <a-govindraju@...com>,
Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc: Vignesh Raghavendra <vigneshr@...com>,
Kishon Vijay Abraham I <kishon@...com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Sven Peter <sven@...npeter.dev>,
Alyssa Rosenzweig <alyssa@...enzweig.io>,
Hector Martin <marcan@...can.st>,
Saranya Gopal <saranya.gopal@...el.com>,
Jens Axboe <axboe@...nel.dk>, linux-usb@...r.kernel.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH RFC] usb: typec: tipd: Add support for polling interrupts
status when interrupt line is not connected
Hi Aswath,
On 13/04/2022 13:02, Aswath Govindraju wrote:
> Hi Heikki,
>
> On 13/04/22 15:04, Heikki Krogerus wrote:
>> Hi Aswath,
>>
>> On Tue, Apr 12, 2022 at 08:20:58PM +0530, Aswath Govindraju wrote:
>>> In some cases the interrupt line from the pd controller may not be
>>> connected. In these cases, poll the status of various events.
>>
>> Well, if the alert/interrupt line is not connected anywhere, then
>> polling is the only way to go. I'm fine with that, but the driver
>> really should be told that there is no interrupt. Using polling
>> whenever request_threaded_irq() returns -EINVAL is wrong. We really
>> should not even attempt to request the interrupt if there is no
>> interrupt for the device.
>>
>> Isn't there any way you can get that information from DT? Or how is
>> the device enumerated in your case?
>>
>
> Would checking if (client->irq) field is populated, to decide between
> polling and interrupts be a good approach?
'interrupt' and 'interrupt-names' are required properties in DT binding doc
Documentation/devicetree/bindings/usb/ti,tps6598x.yaml
You will need to add a new property to indicate that polling mode must be used
and then interrupt properties become optional.
>
> I am sorry but I did not understand what you meant by device getting
> enumerated. The device is on an I2C bus and gets enumerated based on the
> I2C address provided. The device does not have I2C_IRQ line connected,
> in my case.
I think he meant whether you are using device tree or something else.
>
> Thanks,
> Aswath
>
>>> Suggested-by: Roger Quadros <rogerq@...nel.org>
>>> Signed-off-by: Aswath Govindraju <a-govindraju@...com>
>>> ---
>>> drivers/usb/typec/tipd/core.c | 90 ++++++++++++++++++++++++++++++++---
>>> 1 file changed, 83 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/usb/typec/tipd/core.c b/drivers/usb/typec/tipd/core.c
>>> index 16b4560216ba..fa52d2067d6d 100644
>>> --- a/drivers/usb/typec/tipd/core.c
>>> +++ b/drivers/usb/typec/tipd/core.c
>>> @@ -15,6 +15,8 @@
>>> #include <linux/interrupt.h>
>>> #include <linux/usb/typec.h>
>>> #include <linux/usb/role.h>
>>> +#include <linux/workqueue.h>
>>> +#include <linux/devm-helpers.h>
>>>
>>> #include "tps6598x.h"
>>> #include "trace.h"
>>> @@ -93,6 +95,8 @@ struct tps6598x {
>>> struct power_supply *psy;
>>> struct power_supply_desc psy_desc;
>>> enum power_supply_usb_type usb_type;
>>> +
>>> + struct delayed_work wq_poll;
>>> };
>>>
>>> static enum power_supply_property tps6598x_psy_props[] = {
>>> @@ -473,9 +477,8 @@ static void tps6598x_handle_plug_event(struct tps6598x *tps, u32 status)
>>> }
>>> }
>>>
>>> -static irqreturn_t cd321x_interrupt(int irq, void *data)
>>> +static int cd321x_handle_interrupt_status(struct tps6598x *tps)
>>> {
>>> - struct tps6598x *tps = data;
>>> u64 event;
>>> u32 status;
>>> int ret;
>>> @@ -513,14 +516,45 @@ static irqreturn_t cd321x_interrupt(int irq, void *data)
>>> err_unlock:
>>> mutex_unlock(&tps->lock);
>>>
>>> + if (ret)
>>> + return ret;
>>> +
>>> if (event)
>>> - return IRQ_HANDLED;
>>> - return IRQ_NONE;
>>> + return 0;
>>> + return 1;
>>> }
>>>
>>> -static irqreturn_t tps6598x_interrupt(int irq, void *data)
>>> +static irqreturn_t cd321x_interrupt(int irq, void *data)
>>> {
>>> struct tps6598x *tps = data;
>>> + int ret;
>>> +
>>> + ret = cd321x_handle_interrupt_status(tps);
>>> + if (ret)
>>> + return IRQ_NONE;
>>> + return IRQ_HANDLED;
>>> +}
>>> +
>>> +/* Time interval for Polling */
>>> +#define POLL_INTERVAL 500 /* msecs */
>>> +static void cd321x_poll_work(struct work_struct *work)
>>> +{
>>> + struct tps6598x *tps = container_of(to_delayed_work(work),
>>> + struct tps6598x, wq_poll);
>>> + int ret;
>>> +
>>> + ret = cd321x_handle_interrupt_status(tps);
>>> + /*
>>> + * If there is an error while reading the interrupt registers
>>> + * then stop polling else, schedule another poll work item
>>> + */
>>> + if (!(ret < 0))
>>> + queue_delayed_work(system_power_efficient_wq,
>>> + &tps->wq_poll, msecs_to_jiffies(POLL_INTERVAL));
>>> +}
>>> +
>>> +static int tps6598x_handle_interrupt_status(struct tps6598x *tps)
>>> +{
>>> u64 event1;
>>> u64 event2;
>>> u32 status;
>>> @@ -561,9 +595,39 @@ static irqreturn_t tps6598x_interrupt(int irq, void *data)
>>> err_unlock:
>>> mutex_unlock(&tps->lock);
>>>
>>> + if (ret)
>>> + return ret;
>>> +
>>> if (event1 | event2)
>>> - return IRQ_HANDLED;
>>> - return IRQ_NONE;
>>> + return 0;
>>> + return 1;
>>> +}
>>> +
>>> +static irqreturn_t tps6598x_interrupt(int irq, void *data)
>>> +{
>>> + struct tps6598x *tps = data;
>>> + int ret;
>>> +
>>> + ret = tps6598x_handle_interrupt_status(tps);
>>> + if (ret)
>>> + return IRQ_NONE;
>>> + return IRQ_HANDLED;
>>> +}
>>> +
>>> +static void tps6598x_poll_work(struct work_struct *work)
>>> +{
>>> + struct tps6598x *tps = container_of(to_delayed_work(work),
>>> + struct tps6598x, wq_poll);
>>> + int ret;
>>> +
>>> + ret = tps6598x_handle_interrupt_status(tps);
>>> + /*
>>> + * If there is an error while reading the interrupt registers
>>> + * then stop polling else, schedule another poll work item
>>> + */
>>> + if (!(ret < 0))
>>> + queue_delayed_work(system_power_efficient_wq,
>>> + &tps->wq_poll, msecs_to_jiffies(POLL_INTERVAL));
>>> }
>>>
>>> static int tps6598x_check_mode(struct tps6598x *tps)
>>> @@ -704,6 +768,7 @@ static int devm_tps6598_psy_register(struct tps6598x *tps)
>>> static int tps6598x_probe(struct i2c_client *client)
>>> {
>>> irq_handler_t irq_handler = tps6598x_interrupt;
>>> + work_func_t work_poll_handler = tps6598x_poll_work;
>>> struct device_node *np = client->dev.of_node;
>>> struct typec_capability typec_cap = { };
>>> struct tps6598x *tps;
>>> @@ -748,6 +813,7 @@ static int tps6598x_probe(struct i2c_client *client)
>>> APPLE_CD_REG_INT_PLUG_EVENT;
>>>
>>> irq_handler = cd321x_interrupt;
>>> + work_poll_handler = cd321x_poll_work;
>>> } else {
>>> /* Enable power status, data status and plug event interrupts */
>>> mask1 = TPS_REG_INT_POWER_STATUS_UPDATE |
>>> @@ -846,6 +912,16 @@ static int tps6598x_probe(struct i2c_client *client)
>>> irq_handler,
>>> IRQF_SHARED | IRQF_ONESHOT,
>>> dev_name(&client->dev), tps);
>>> + if (ret == -EINVAL) {
>>> + dev_warn(&client->dev, "Unable to find the interrupt, switching to polling\n");
>>> + ret = devm_delayed_work_autocancel(tps->dev, &tps->wq_poll, work_poll_handler);
>>> + if (ret)
>>> + dev_err(&client->dev, "error while initializing workqueue\n");
>>> + else
>>> + queue_delayed_work(system_power_efficient_wq, &tps->wq_poll,
>>> + msecs_to_jiffies(POLL_INTERVAL));
>>> + }
>>> +
>>> if (ret) {
>>> tps6598x_disconnect(tps, 0);
>>> typec_unregister_port(tps->port);
>>
>> thanks,
>>
>
>
--
cheers,
-roger
Powered by blists - more mailing lists