From 88a5ed5970f498cf0e29789c1ac188fa967eeb2c Mon Sep 17 00:00:00 2001 From: Guru Das Srinagesh Date: Wed, 11 Mar 2020 10:46:00 -0700 Subject: [PATCH 1/6] soc: altmode: Update client APIs, notifier bookkeeping Client API changes: - Update client APIs to align them more closely with upstream conventions. - In particular, change the link between altmode device and client from the altmode device's name string to a phandle to the altmode device followed by the port index of the client. - Instead of using solely the client's SVID as the key for idr_alloc, include port_index as well because the unique identifier of a client is the (SVID, port_index) tuple. Updates to client notifying mechanism: - Get rid of the standard notifier APIs in favour of a simpler callback function-based mechanism to notify clients of probe completion. - In addition, if clients probe after altmode finishes probing, they will not get notified of probe completion as this is done only once at the end of altmode_probe(). Fix this so that if a client registers for probe completion notifications after altmode probes, they get notified immediately. While at it, get rid of the global list of altmode devices (amdev_list) as there will only be a single altmode device in the system as per design. Change-Id: Iea450d30d9cc94671f505b7ff0c6e98b0b8a4467 Signed-off-by: Guru Das Srinagesh --- drivers/soc/qcom/altmode-glink.c | 262 ++++++++++++++----------- include/linux/soc/qcom/altmode-glink.h | 16 +- 2 files changed, 156 insertions(+), 122 deletions(-) diff --git a/drivers/soc/qcom/altmode-glink.c b/drivers/soc/qcom/altmode-glink.c index bca5359e8704..1f897229e727 100644 --- a/drivers/soc/qcom/altmode-glink.c +++ b/drivers/soc/qcom/altmode-glink.c @@ -26,6 +26,12 @@ #define ALTMODE_NAME_MAX_LEN 10 +#define MAX_NUM_PORTS 4 + +#define IDR_KEY_GEN(svid, ind) (((svid) << 8) | (ind)) +#define IDR_KEY(client) \ + IDR_KEY_GEN((client)->data.svid, (client)->port_index) + struct usbc_notify_ind_msg { struct pmic_glink_hdr hdr; u8 payload[NOTIFY_PAYLOAD_SIZE]; @@ -51,7 +57,6 @@ struct usbc_write_buffer_req_msg { * @client_list: Linked list head keeping track of this device's clients * @pan_en_sent: Flag to ensure PAN Enable msg is sent only once * @send_pan_en_work: To schedule the sending of the PAN Enable message - * @probe_notifier: Inform clients of altmode probe completion */ struct altmode_dev { struct device *dev; @@ -63,7 +68,6 @@ struct altmode_dev { struct list_head client_list; atomic_t pan_en_sent; struct delayed_work send_pan_en_work; - struct raw_notifier_head probe_notifier; }; /** @@ -87,45 +91,33 @@ struct altmode_client { * Linked list node to keep track of altmode clients who, by design, * register with the altmode framework before altmode probes. * - * @amdev_name: Name of the altmode device client wants to bind to - * @nb: Client's notifier block * @node: Linked list node for probe_notify_list + * @amdev_node: device_node of the altmode device of the client + * @cb: Client's probe completion callback function + * @priv: Pointer to client's driver data struct */ struct probe_notify_node { - char *amdev_name; - struct notifier_block *nb; struct list_head node; + struct device_node *amdev_node; + void (*cb)(void *priv); + void *priv; }; -/** - * struct list_head amdev_list - * List of altmode devices currently using this driver. - */ -static LIST_HEAD(amdev_list); -static DEFINE_MUTEX(amdev_lock); - /** * struct list_head probe_notify_list * List of altmode clients that register to get notified upon probe - * completion. This list is traversed and clients are removed from this - * list after they have been registered with the altmode device's - * raw_notifier_head. + * completion. */ static LIST_HEAD(probe_notify_list); static DEFINE_MUTEX(notify_lock); -static struct altmode_dev *get_amdev_from_dev(struct device *dev) +static struct altmode_dev *to_altmode_device(struct device_node *amdev_node) { - struct altmode_dev *pos, *tmp; + struct platform_device *altmode_pdev; - mutex_lock(&amdev_lock); - list_for_each_entry_safe(pos, tmp, &amdev_list, d_node) { - if (pos->dev == dev) { - mutex_unlock(&amdev_lock); - return pos; - } - } - mutex_unlock(&amdev_lock); + altmode_pdev = of_find_device_by_node(amdev_node); + if (altmode_pdev) + return platform_get_drvdata(altmode_pdev); return NULL; } @@ -157,33 +149,60 @@ static int __altmode_send_data(struct altmode_dev *amdev, void *data, /** * altmode_register_notifier() - * Register to be notified when altmode probes. + * Register to be notified when altmode device probes. * - * @amdev_name: The altmode device being registered with by client - * @nb: Notifier block to get notified upon probe completion + * @client_dev: Client device pointer + * @cb: Callback function to execute when altmode device of + * interest probes successfully + * @priv: Client's private data which is passed back when cb() is + * called * * Returns: 0 upon success, negative upon errors. */ -int altmode_register_notifier(const char *amdev_name, struct notifier_block *nb) +int altmode_register_notifier(struct device *client_dev, void (*cb)(void *), + void *priv) { + int rc; struct probe_notify_node *notify_node; + struct device_node *amdev_node; + struct of_phandle_args pargs; + struct altmode_dev *amdev; - if (!amdev_name || !nb) + if (!client_dev || !cb || !priv) return -EINVAL; - notify_node = kzalloc(sizeof(*notify_node), GFP_KERNEL); - if (!notify_node) - return -ENOMEM; - - notify_node->nb = nb; - notify_node->amdev_name = kstrdup(amdev_name, GFP_KERNEL); - if (!notify_node->amdev_name) { - kfree(notify_node); - return -ENOMEM; + rc = of_parse_phandle_with_args(client_dev->of_node, + "qcom,altmode-dev", "#altmode-cells", 0, &pargs); + if (rc) { + dev_err(client_dev, "Error parsing qcom,altmode-dev property: %d\n", + rc); + return rc; } + amdev_node = pargs.np; + mutex_lock(¬ify_lock); - list_add(¬ify_node->node, &probe_notify_list); + amdev = to_altmode_device(amdev_node); + if (amdev) { + /* + * If altmode device has probed already, notify + * immediately. + */ + of_node_put(amdev_node); + cb(priv); + } else { + notify_node = kzalloc(sizeof(*notify_node), GFP_KERNEL); + if (!notify_node) { + mutex_unlock(¬ify_lock); + return -ENOMEM; + } + + notify_node->cb = cb; + notify_node->priv = priv; + notify_node->amdev_node = amdev_node; + + list_add(¬ify_node->node, &probe_notify_list); + } mutex_unlock(¬ify_lock); return 0; @@ -194,24 +213,45 @@ EXPORT_SYMBOL(altmode_register_notifier); * altmode_deregister_notifier() * Deregister probe completion notifier. * - * @client: The altmode client obtained during registration - * @nb: Notifier block used for registration + * @client_dev: Client device pointer + * @priv: Client's private data * * Returns: 0 upon success, negative upon errors. */ -int altmode_deregister_notifier(struct altmode_client *client, - struct notifier_block *nb) +int altmode_deregister_notifier(struct device *client_dev, void *priv) { - struct altmode_dev *amdev; + int rc; + struct device_node *amdev_node; + struct probe_notify_node *pos, *tmp; + struct of_phandle_args pargs; - if (!client || !nb) + if (!client_dev) return -EINVAL; - amdev = client->amdev; - if (!amdev) - return -ENODEV; + rc = of_parse_phandle_with_args(client_dev->of_node, + "qcom,altmode-dev", "#altmode-cells", 0, &pargs); + if (rc) { + dev_err(client_dev, "Error parsing qcom,altmode-dev property: %d\n", + rc); + return rc; + } - return raw_notifier_chain_unregister(&amdev->probe_notifier, nb); + amdev_node = pargs.np; + + mutex_lock(¬ify_lock); + list_for_each_entry_safe(pos, tmp, &probe_notify_list, node) { + if (pos->amdev_node == amdev_node && pos->priv == priv) { + of_node_put(pos->amdev_node); + list_del(&pos->node); + kfree(pos); + break; + } + } + mutex_unlock(¬ify_lock); + + of_node_put(amdev_node); + + return 0; } EXPORT_SYMBOL(altmode_deregister_notifier); @@ -240,7 +280,7 @@ EXPORT_SYMBOL(altmode_send_data); * Register with altmode to receive PMIC GLINK messages from remote * subsystem. * - * @dev: Device of the parent altmode (platform) device + * @client_dev: Client device pointer * @client_data: Details identifying altmode client uniquely * * Returns: Valid altmode client pointer upon success, ERR_PTRs @@ -248,32 +288,54 @@ EXPORT_SYMBOL(altmode_send_data); * * Notes: client_data should contain a unique SVID. */ -struct altmode_client *altmode_register_client(struct device *dev, +struct altmode_client *altmode_register_client(struct device *client_dev, const struct altmode_client_data *client_data) { - int rc; + int rc, key; struct altmode_dev *amdev; + struct of_phandle_args pargs; + struct device_node *amdev_node; struct altmode_client *amclient; - if (!dev || !dev->parent) - return ERR_PTR(-ENODEV); + if (!client_dev || !client_data) + return ERR_PTR(-EINVAL); if (!client_data->name || !client_data->priv || !client_data->callback || !client_data->svid) return ERR_PTR(-EINVAL); - amdev = get_amdev_from_dev(dev); - if (!amdev) { - pr_err("No alt mode device exists for %s\n", client_data->name); - return ERR_PTR(-ENODEV); + rc = of_parse_phandle_with_args(client_dev->of_node, + "qcom,altmode-dev", "#altmode-cells", 0, &pargs); + if (rc) { + dev_err(client_dev, "Error parsing qcom,altmode-dev property: %d\n", + rc); + return ERR_PTR(rc); } + if (pargs.args_count != 1) { + dev_err(client_dev, "Error in port_index specification\n"); + return ERR_PTR(-EINVAL); + } + + if (pargs.args[0] >= MAX_NUM_PORTS) { + dev_err(client_dev, "Invalid port_index: %d, max is %d\n", + pargs.args[0], MAX_NUM_PORTS - 1); + return ERR_PTR(-EINVAL); + } + + amdev_node = pargs.np; + + amdev = to_altmode_device(amdev_node); + of_node_put(amdev_node); + if (!amdev) + return ERR_PTR(-EPROBE_DEFER); + amclient = kzalloc(sizeof(*amclient), GFP_KERNEL); if (!amclient) return ERR_PTR(-ENOMEM); amclient->amdev = amdev; - amclient->port_index = U8_MAX; /* invalid */ + amclient->port_index = pargs.args[0]; amclient->data.svid = client_data->svid; amclient->data.priv = client_data->priv; amclient->data.callback = client_data->callback; @@ -284,8 +346,8 @@ struct altmode_client *altmode_register_client(struct device *dev, } mutex_lock(&amdev->client_lock); - rc = idr_alloc(&amdev->client_idr, amclient, amclient->data.svid, - amclient->data.svid + 1, GFP_KERNEL); + key = IDR_KEY(amclient); + rc = idr_alloc(&amdev->client_idr, amclient, key, key + 1, GFP_KERNEL); if (rc < 0) { pr_err("Error in allocating idr for client %s: %d\n", client_data->name, rc); @@ -330,7 +392,7 @@ int altmode_deregister_client(struct altmode_client *client) amdev = client->amdev; mutex_lock(&amdev->client_lock); - idr_remove(&amdev->client_idr, client->data.svid); + idr_remove(&amdev->client_idr, IDR_KEY(client)); list_for_each_entry_safe(pos, tmp, &amdev->client_list, c_node) { if (pos == client) @@ -429,64 +491,47 @@ static int altmode_callback(void *priv, void *data, size_t len) port_index = notify_msg->payload[0]; mutex_lock(&amdev->client_lock); - amclient = idr_find(&amdev->client_idr, svid); + amclient = idr_find(&amdev->client_idr, IDR_KEY_GEN(svid, port_index)); mutex_unlock(&amdev->client_lock); if (op == USBC_NOTIFY_IND) { if (!amclient) { - pr_debug("No client associated with SVID %#x\n", svid); + pr_debug("No client associated with SVID %#x port %u\n", + svid, port_index); altmode_send_ack(amdev, port_index); return 0; } - if (amclient->port_index == U8_MAX) - amclient->port_index = port_index; - pr_debug("Payload: %*ph\n", NOTIFY_PAYLOAD_SIZE, notify_msg->payload); amclient->data.callback(amclient->data.priv, notify_msg->payload, len); - } return 0; } -static void altmode_gather_clients(struct altmode_dev *amdev) +static void altmode_notify_clients(struct altmode_dev *amdev) { + struct altmode_dev *pos_amdev; struct probe_notify_node *pos, *tmp; mutex_lock(¬ify_lock); list_for_each_entry_safe(pos, tmp, &probe_notify_list, node) { - if (!strcmp(pos->amdev_name, amdev->name)) { - raw_notifier_chain_register(&amdev->probe_notifier, - pos->nb); - /* - * Client's nb has been added to amdev's notifier, so - * it may be removed from the global list. - */ + pos_amdev = to_altmode_device(pos->amdev_node); + if (!pos_amdev) + continue; + + if (pos_amdev == amdev) { + pos->cb(pos->priv); + of_node_put(pos->amdev_node); list_del(&pos->node); - kfree(pos->amdev_name); kfree(pos); } } mutex_unlock(¬ify_lock); } -static void altmode_notify_clients(struct altmode_dev *amdev, - struct platform_device *pdev) -{ - altmode_gather_clients(amdev); - raw_notifier_call_chain(&amdev->probe_notifier, 0, pdev); -} - -static void altmode_device_add(struct altmode_dev *amdev) -{ - mutex_lock(&amdev_lock); - list_add(&amdev->d_node, &amdev_list); - mutex_unlock(&amdev_lock); -} - static int altmode_probe(struct platform_device *pdev) { int rc; @@ -515,8 +560,6 @@ static int altmode_probe(struct platform_device *pdev) amdev->dev = dev; strlcpy(amdev->name, str, ALTMODE_NAME_MAX_LEN); - RAW_INIT_NOTIFIER_HEAD(&amdev->probe_notifier); - mutex_init(&amdev->client_lock); idr_init(&amdev->client_idr); INIT_DELAYED_WORK(&amdev->send_pan_en_work, altmode_send_pan_en); @@ -541,8 +584,7 @@ static int altmode_probe(struct platform_device *pdev) platform_set_drvdata(pdev, amdev); - altmode_device_add(amdev); - altmode_notify_clients(amdev, pdev); + altmode_notify_clients(amdev); return 0; @@ -551,36 +593,30 @@ static int altmode_probe(struct platform_device *pdev) return rc; } -static void altmode_device_remove(struct altmode_dev *amdev) -{ - struct altmode_dev *pos, *tmp; - - atomic_set(&amdev->pan_en_sent, 0); - - mutex_lock(&amdev_lock); - list_for_each_entry_safe(pos, tmp, &amdev_list, d_node) { - if (pos == amdev) - list_del(&pos->d_node); - } - mutex_unlock(&amdev_lock); -} - static int altmode_remove(struct platform_device *pdev) { int rc; struct altmode_dev *amdev = platform_get_drvdata(pdev); struct altmode_client *client, *tmp; + struct probe_notify_node *npos, *ntmp; cancel_delayed_work_sync(&amdev->send_pan_en_work); idr_destroy(&amdev->client_idr); + atomic_set(&amdev->pan_en_sent, 0); + + mutex_lock(¬ify_lock); + list_for_each_entry_safe(npos, ntmp, &probe_notify_list, node) { + of_node_put(npos->amdev_node); + list_del(&npos->node); + kfree(npos); + } + mutex_unlock(¬ify_lock); mutex_lock(&amdev->client_lock); list_for_each_entry_safe(client, tmp, &amdev->client_list, c_node) list_del(&client->c_node); mutex_unlock(&amdev->client_lock); - altmode_device_remove(amdev); - rc = pmic_glink_unregister_client(amdev->pgclient); if (rc < 0) dev_err(amdev->dev, "Error in pmic_glink de-registration: %d\n", diff --git a/include/linux/soc/qcom/altmode-glink.h b/include/linux/soc/qcom/altmode-glink.h index f93c1983c622..4f5d0e44b1ff 100644 --- a/include/linux/soc/qcom/altmode-glink.h +++ b/include/linux/soc/qcom/altmode-glink.h @@ -38,13 +38,11 @@ struct altmode_pan_ack_msg { #if IS_ENABLED(CONFIG_QTI_ALTMODE_GLINK) -struct notifier_block; struct device; -int altmode_register_notifier(const char *amdev_name, - struct notifier_block *nb); -int altmode_deregister_notifier(struct altmode_client *client, - struct notifier_block *nb); +int altmode_register_notifier(struct device *client_dev, void (*cb)(void *), + void *priv); +int altmode_deregister_notifier(struct device *client_dev, void *priv); struct altmode_client *altmode_register_client(struct device *dev, const struct altmode_client_data *client_data); int altmode_deregister_client(struct altmode_client *client); @@ -52,14 +50,14 @@ int altmode_send_data(struct altmode_client *client, void *data, size_t len); #else -static inline int altmode_register_notifier(const char *amdev_name, - struct notifier_block *nb) +static inline int altmode_register_notifier(struct device *client_dev, + void (*cb)(void *), void *priv) { return -ENODEV; } -static inline int altmode_deregister_notifier(struct altmode_client *client, - struct notifier_block *nb) +static inline int altmode_deregister_notifier(struct device *client_dev, + void *priv) { return -ENODEV; } From 51467ac1d6e31d7f5940ed883d9f32e2fb4c90aa Mon Sep 17 00:00:00 2001 From: Guru Das Srinagesh Date: Wed, 18 Mar 2020 21:05:31 -0700 Subject: [PATCH 2/6] soc: altmode: Remove altmode device name Remove the device tree property "qcom,altmode-name" and the logic it feeds as it was meant to help in the cases when there are multiple altmode devices. Since there is going to be only one altmode device as per design, this is no longer needed. Change-Id: Ife15e39becee5f4ff03bdcc56ff62ae812fe2726 Signed-off-by: Guru Das Srinagesh --- drivers/soc/qcom/altmode-glink.c | 19 ------------------- 1 file changed, 19 deletions(-) diff --git a/drivers/soc/qcom/altmode-glink.c b/drivers/soc/qcom/altmode-glink.c index 1f897229e727..e721cc197f76 100644 --- a/drivers/soc/qcom/altmode-glink.c +++ b/drivers/soc/qcom/altmode-glink.c @@ -24,8 +24,6 @@ #define USBC_CMD_WRITE_REQ 0x15 #define USBC_NOTIFY_IND 0x16 -#define ALTMODE_NAME_MAX_LEN 10 - #define MAX_NUM_PORTS 4 #define IDR_KEY_GEN(svid, ind) (((svid) << 8) | (ind)) @@ -49,7 +47,6 @@ struct usbc_write_buffer_req_msg { * Definition of an altmode device. * * @dev: Altmode parent device for all client devices - * @name: Short descriptive name of this altmode device * @pgclient: PMIC GLINK client to talk to remote subsystem * @client_idr: idr list for altmode clients * @client_lock: mutex protecting changes to client_idr @@ -60,7 +57,6 @@ struct usbc_write_buffer_req_msg { */ struct altmode_dev { struct device *dev; - char name[ALTMODE_NAME_MAX_LEN]; struct pmic_glink_client *pgclient; struct idr client_idr; struct mutex client_lock; @@ -535,7 +531,6 @@ static void altmode_notify_clients(struct altmode_dev *amdev) static int altmode_probe(struct platform_device *pdev) { int rc; - const char *str = NULL; struct altmode_dev *amdev; struct pmic_glink_client_data pgclient_data = { }; struct device *dev = &pdev->dev; @@ -544,21 +539,7 @@ static int altmode_probe(struct platform_device *pdev) if (!amdev) return -ENOMEM; - rc = of_property_read_string(dev->of_node, "qcom,altmode-name", - &str); - if (rc < 0) { - dev_err(dev, "No altmode device name specified: %d\n", rc); - return rc; - } - - if (!str || (strlen(str) >= ALTMODE_NAME_MAX_LEN) || - !str_has_prefix(str, "altmode_")) { - dev_err(dev, "Incorrect altmode device name format\n"); - return -EINVAL; - } - amdev->dev = dev; - strlcpy(amdev->name, str, ALTMODE_NAME_MAX_LEN); mutex_init(&amdev->client_lock); idr_init(&amdev->client_idr); From 5063ec589dca71b87029f6a38385b89639663103 Mon Sep 17 00:00:00 2001 From: Guru Das Srinagesh Date: Mon, 24 Feb 2020 17:53:47 -0800 Subject: [PATCH 3/6] soc: altmode: Add debugfs nodes to send PAN EN and PAN ACK Add debugfs nodes to send Pin Assignment Notifications Enable (PAN EN) and Acknowledgment (ACK) commands manually if needed, to aid in debugging situations where it is unclear whether clients are sending these commands synchronously as per design. Change-Id: I7a68d676df86cbf6952158202c8e4958db032d5b Signed-off-by: Guru Das Srinagesh --- drivers/soc/qcom/altmode-glink.c | 93 ++++++++++++++++++++++++++++++++ 1 file changed, 93 insertions(+) diff --git a/drivers/soc/qcom/altmode-glink.c b/drivers/soc/qcom/altmode-glink.c index e721cc197f76..fefd672fe881 100644 --- a/drivers/soc/qcom/altmode-glink.c +++ b/drivers/soc/qcom/altmode-glink.c @@ -5,6 +5,7 @@ #define pr_fmt(fmt) "altmode-glink: %s: " fmt, __func__ +#include #include #include #include @@ -54,6 +55,7 @@ struct usbc_write_buffer_req_msg { * @client_list: Linked list head keeping track of this device's clients * @pan_en_sent: Flag to ensure PAN Enable msg is sent only once * @send_pan_en_work: To schedule the sending of the PAN Enable message + * @debugfs_dir: Dentry for debugfs directory "altmode" */ struct altmode_dev { struct device *dev; @@ -64,6 +66,7 @@ struct altmode_dev { struct list_head client_list; atomic_t pan_en_sent; struct delayed_work send_pan_en_work; + struct dentry *debugfs_dir; }; /** @@ -528,6 +531,86 @@ static void altmode_notify_clients(struct altmode_dev *amdev) mutex_unlock(¬ify_lock); } +#ifdef CONFIG_QTI_PMIC_GLINK_CLIENT_DEBUG +static int pan_en_write(void *data, u64 val) +{ + struct altmode_dev *amdev = data; + + schedule_delayed_work(&amdev->send_pan_en_work, + msecs_to_jiffies(20)); + return 0; +} +DEFINE_DEBUGFS_ATTRIBUTE(pan_en_fops, NULL, pan_en_write, "%llu\n"); + +static int send_ack_write(void *data, u64 val) +{ + int rc; + struct altmode_dev *amdev = data; + struct altmode_pan_ack_msg ack; + + if (val >= MAX_NUM_PORTS) + return -EINVAL; + + ack.cmd_type = ALTMODE_PAN_ACK; + ack.port_index = val; + + rc = __altmode_send_data(amdev, &ack, sizeof(ack)); + if (rc < 0) { + dev_err(amdev->dev, "port %d: Failed sending PAN ACK: %llu\n", + val, rc); + return rc; + } + + dev_dbg(amdev->dev, "port %llu: Sent PAN ACK via debugfs\n", val); + + return 0; +} +DEFINE_DEBUGFS_ATTRIBUTE(send_ack_fops, NULL, send_ack_write, "%llu\n"); + +static int altmode_setup_debugfs(struct altmode_dev *amdev) +{ + int rc; + struct dentry *am_dir, *file; + + am_dir = debugfs_create_dir("altmode", NULL); + if (IS_ERR(am_dir)) { + rc = PTR_ERR(am_dir); + dev_err(amdev->dev, "Failed to create altmode directory: %d\n", + rc); + return rc; + } + + file = debugfs_create_file_unsafe("send_pan_en", 0200, am_dir, amdev, + &pan_en_fops); + if (IS_ERR(file)) { + rc = PTR_ERR(file); + dev_err(amdev->dev, "Failed to create send_pan_en: %d\n", rc); + goto error; + } + + file = debugfs_create_file_unsafe("send_pan_ack", 0200, am_dir, + amdev, &send_ack_fops); + if (IS_ERR(file)) { + rc = PTR_ERR(file); + dev_err(amdev->dev, "Failed to create send_pan_ack: %d\n", rc); + goto error; + } + + amdev->debugfs_dir = am_dir; + + return 0; + +error: + debugfs_remove_recursive(am_dir); + return rc; +} +#else +static int altmode_setup_debugfs(struct altmode_dev *amdev) +{ + return 0; +} +#endif + static int altmode_probe(struct platform_device *pdev) { int rc; @@ -565,10 +648,18 @@ static int altmode_probe(struct platform_device *pdev) platform_set_drvdata(pdev, amdev); + rc = altmode_setup_debugfs(amdev); + if (rc < 0) { + dev_err(amdev->dev, "Failed to create debugfs: %d\n", rc); + goto unreg_pmic_glink; + } + altmode_notify_clients(amdev); return 0; +unreg_pmic_glink: + pmic_glink_unregister_client(amdev->pgclient); error_register: idr_destroy(&amdev->client_idr); return rc; @@ -581,6 +672,8 @@ static int altmode_remove(struct platform_device *pdev) struct altmode_client *client, *tmp; struct probe_notify_node *npos, *ntmp; + debugfs_remove_recursive(amdev->debugfs_dir); + cancel_delayed_work_sync(&amdev->send_pan_en_work); idr_destroy(&amdev->client_idr); atomic_set(&amdev->pan_en_sent, 0); From 011cf3718b5e1f9c11dc1db9970dcfaa1853d442 Mon Sep 17 00:00:00 2001 From: Guru Das Srinagesh Date: Thu, 26 Mar 2020 13:13:07 -0700 Subject: [PATCH 4/6] soc: altmode: Detect when remote subsys fails to respond The remote subsystem acknowledges all Altmode PMIC GLINK messages sent to it. Print an error message anytime it fails to do this. In the case when no client is found for a received message, do the sending of the ack in a separate worker thread instead of a plain function call to altmode_send_ack(). This is to avoid a deadlock and consequent timing out in altmode_write(), which will happen in the following scenario: - A message is received for a nonexistent client. - altmode_send_ack() is called from within altmode_callback(). - The PAN ACK is sent to the remote subsystem via pmic_glink_write() in altmode_write() and the remote subsystem replies with an ACK (opcode USBC_CMD_WRITE_REQ). - Subsequently, in altmode_write() we wait for completion timeout, which will happen only if this new ACK message is processed by PMIC GLINK. - PMIC GLINK is waiting for altmode_callback() to return so that it can process the new message. - We are still waiting in altmode_callback() for altmode_send_ack() to complete. - We time out in altmode_write() waiting for the completion to occur. This problem is solved by firing off a worker thread to send the PAN ACK so that altmode_callback() can exit quickly and allow the incoming new ACK to hit the complete() in the USBC_CMD_WRITE_REQ case in altmode_callback(). Also add a couple of minor fixes to some debug messages while we're at it. Change-Id: I3d80b1a7d6cc03b0eba61b5c9cc660ce7d377942 Signed-off-by: Guru Das Srinagesh --- drivers/soc/qcom/altmode-glink.c | 62 ++++++++++++++++++++++++++------ 1 file changed, 52 insertions(+), 10 deletions(-) diff --git a/drivers/soc/qcom/altmode-glink.c b/drivers/soc/qcom/altmode-glink.c index fefd672fe881..9d73c3ca76ae 100644 --- a/drivers/soc/qcom/altmode-glink.c +++ b/drivers/soc/qcom/altmode-glink.c @@ -55,7 +55,10 @@ struct usbc_write_buffer_req_msg { * @client_list: Linked list head keeping track of this device's clients * @pan_en_sent: Flag to ensure PAN Enable msg is sent only once * @send_pan_en_work: To schedule the sending of the PAN Enable message + * @send_pan_ack_work: To schedule the sending of the PAN Ack message * @debugfs_dir: Dentry for debugfs directory "altmode" + * @response_received: To detect remote subsystem response failures + * @ack_port_index: Port index to ack for a nonexistent client */ struct altmode_dev { struct device *dev; @@ -66,7 +69,10 @@ struct altmode_dev { struct list_head client_list; atomic_t pan_en_sent; struct delayed_work send_pan_en_work; + struct delayed_work send_pan_ack_work; struct dentry *debugfs_dir; + struct completion response_received; + u8 ack_port_index; }; /** @@ -110,6 +116,8 @@ struct probe_notify_node { static LIST_HEAD(probe_notify_list); static DEFINE_MUTEX(notify_lock); +static void altmode_send_pan_ack(struct work_struct *work); + static struct altmode_dev *to_altmode_device(struct device_node *amdev_node) { struct platform_device *altmode_pdev; @@ -121,10 +129,28 @@ static struct altmode_dev *to_altmode_device(struct device_node *amdev_node) return NULL; } +#define ALTMODE_WAIT_MS 1000 +static int altmode_write(struct altmode_dev *amdev, void *data, size_t len) +{ + int rc; + + reinit_completion(&amdev->response_received); + rc = pmic_glink_write(amdev->pgclient, data, len); + if (!rc) { + rc = wait_for_completion_timeout(&amdev->response_received, + msecs_to_jiffies(ALTMODE_WAIT_MS)); + rc = rc ? 0 : -ETIMEDOUT; + } + + if (rc) + pr_err("Error in sending message: %d\n", rc); + + return rc; +} + static int __altmode_send_data(struct altmode_dev *amdev, void *data, size_t len) { - int rc; struct usbc_write_buffer_req_msg msg = { { 0 } }; if (len > sizeof(msg.buf)) { @@ -139,11 +165,7 @@ static int __altmode_send_data(struct altmode_dev *amdev, void *data, memcpy(msg.buf, data, len); - rc = pmic_glink_write(amdev->pgclient, &msg, sizeof(msg)); - if (rc < 0) - pr_err("Error in sending message: %d\n", rc); - - return rc; + return altmode_write(amdev, &msg, sizeof(msg)); } /** @@ -433,11 +455,11 @@ static int altmode_send_ack(struct altmode_dev *amdev, u8 port_index) rc = __altmode_send_data(amdev, &ack, sizeof(ack)); if (rc < 0) { - pr_err("port %u: Failed to send PAN ACK\n", port_index); + pr_err("port %u: Failed to send PAN ACK: %d\n", port_index, rc); return rc; } - pr_debug("port %d: Sent PAN ACK\n", port_index); + pr_debug("port %u: Sent PAN ACK\n", port_index); return rc; } @@ -464,6 +486,14 @@ static void altmode_state_cb(void *priv, enum pmic_glink_state state) } } +static void altmode_send_pan_ack(struct work_struct *work) +{ + struct altmode_dev *amdev = container_of(work, struct altmode_dev, + send_pan_ack_work.work); + + altmode_send_ack(amdev, amdev->ack_port_index); +} + #define USBC_NOTIFY_IND_MASK GENMASK(7, 0) #define GET_OP(opcode) (opcode & USBC_NOTIFY_IND_MASK) #define GET_SVID(opcode) (opcode >> 16) @@ -493,11 +523,17 @@ static int altmode_callback(void *priv, void *data, size_t len) amclient = idr_find(&amdev->client_idr, IDR_KEY_GEN(svid, port_index)); mutex_unlock(&amdev->client_lock); - if (op == USBC_NOTIFY_IND) { + switch (op) { + case USBC_CMD_WRITE_REQ: + complete(&amdev->response_received); + break; + case USBC_NOTIFY_IND: if (!amclient) { pr_debug("No client associated with SVID %#x port %u\n", svid, port_index); - altmode_send_ack(amdev, port_index); + amdev->ack_port_index = port_index; + schedule_delayed_work(&amdev->send_pan_ack_work, + msecs_to_jiffies(20)); return 0; } @@ -505,6 +541,9 @@ static int altmode_callback(void *priv, void *data, size_t len) notify_msg->payload); amclient->data.callback(amclient->data.priv, notify_msg->payload, len); + break; + default: + break; } return 0; @@ -626,7 +665,9 @@ static int altmode_probe(struct platform_device *pdev) mutex_init(&amdev->client_lock); idr_init(&amdev->client_idr); + init_completion(&amdev->response_received); INIT_DELAYED_WORK(&amdev->send_pan_en_work, altmode_send_pan_en); + INIT_DELAYED_WORK(&amdev->send_pan_ack_work, altmode_send_pan_ack); INIT_LIST_HEAD(&amdev->d_node); INIT_LIST_HEAD(&amdev->client_list); @@ -675,6 +716,7 @@ static int altmode_remove(struct platform_device *pdev) debugfs_remove_recursive(amdev->debugfs_dir); cancel_delayed_work_sync(&amdev->send_pan_en_work); + cancel_delayed_work_sync(&amdev->send_pan_ack_work); idr_destroy(&amdev->client_idr); atomic_set(&amdev->pan_en_sent, 0); From 12f518596306e97584fb3bae33c4bd6f5ffdea18 Mon Sep 17 00:00:00 2001 From: Guru Das Srinagesh Date: Mon, 22 Jun 2020 09:52:44 -0700 Subject: [PATCH 5/6] soc: qcom: altmode-glink: Execute client callback in work Execute the client's callback in a separate thread to avoid the following deadlock situation: - altmode_callback() calls the client's callback when a valid amclient is found and program execution waits for it to finish. - The client's callback calls altmode_send_data() to send the PAN_ACK. - altmode_send_data() eventually calls altmode_write(), which is waiting for the response_received to be "completed" in altmode_callback(). - For response_received to be "completed", altmode_callback() must finish executing in order to process the USBC_CMD_WRITE_REQ message received from the remote subsystem. - altmode_callback() cannot finish executing until altmode_send_data() returns. - altmode_send_data() times out and passes on the error to the client. Also perform the following: - Add a check to ensure that the payload received from the remote subsystem is at least 9 bytes long. - Prevent out of bounds access for USBC_CMD_WRITE_REQ message (which is of a different format than struct usbc_notify_ind_msg) by moving the port_index reading to only the USBC_NOTIFY_IND message in altmode_callback(). CRs-Fixed: 2713590 Fixes: 6c0ffc67e574 ("soc: altmode: Detect when remote subsys fails to respond") Change-Id: I0f2c6ce0f843c43c9e7e301be9ff3ad913310f62 Signed-off-by: Guru Das Srinagesh --- drivers/soc/qcom/altmode-glink.c | 55 +++++++++++++++++++++++++------- 1 file changed, 44 insertions(+), 11 deletions(-) diff --git a/drivers/soc/qcom/altmode-glink.c b/drivers/soc/qcom/altmode-glink.c index 9d73c3ca76ae..9dabf6a7d3e5 100644 --- a/drivers/soc/qcom/altmode-glink.c +++ b/drivers/soc/qcom/altmode-glink.c @@ -19,6 +19,7 @@ #define MSG_OWNER_USBC_PAN 32780 #define MSG_TYPE_REQ_RESP 1 +#define MIN_PAYLOAD_SIZE 9 #define NOTIFY_PAYLOAD_SIZE 16 #define USBC_WRITE_BUFFER_SIZE 8 @@ -83,12 +84,20 @@ struct altmode_dev { * @data: Supplied by client driver during registration * @c_node: Linked list node for parent altmode device's client list * @port_index: Type-C port index assigned by remote subystem + * + * Note: The following members are for internal use only, clients should not + * use them. + * + * @client_cb_work: Work to run client callback + * @msg: Latest notify msg stashed to be retrieved in cb_work */ struct altmode_client { struct altmode_dev *amdev; struct altmode_client_data data; struct list_head c_node; u8 port_index; + struct work_struct client_cb_work; + u8 msg[NOTIFY_PAYLOAD_SIZE]; }; /** @@ -296,6 +305,15 @@ int altmode_send_data(struct altmode_client *client, void *data, } EXPORT_SYMBOL(altmode_send_data); +static void client_cb_work(struct work_struct *work) +{ + struct altmode_client *amclient = container_of(work, + struct altmode_client, client_cb_work); + + amclient->data.callback(amclient->data.priv, amclient->msg, + sizeof(amclient->msg)); +} + /** * altmode_register_client() * Register with altmode to receive PMIC GLINK messages from remote @@ -360,6 +378,7 @@ struct altmode_client *altmode_register_client(struct device *client_dev, amclient->data.svid = client_data->svid; amclient->data.priv = client_data->priv; amclient->data.callback = client_data->callback; + INIT_WORK(&amclient->client_cb_work, client_cb_work); amclient->data.name = kstrdup(client_data->name, GFP_KERNEL); if (!amclient->data.name) { kfree(amclient); @@ -412,6 +431,8 @@ int altmode_deregister_client(struct altmode_client *client) amdev = client->amdev; + cancel_work_sync(&client->client_cb_work); + mutex_lock(&amdev->client_lock); idr_remove(&amdev->client_idr, IDR_KEY(client)); @@ -501,10 +522,11 @@ static void altmode_send_pan_ack(struct work_struct *work) static int altmode_callback(void *priv, void *data, size_t len) { u16 svid, op; - struct usbc_notify_ind_msg *notify_msg = data; - struct pmic_glink_hdr *hdr = ¬ify_msg->hdr; + struct usbc_notify_ind_msg *notify_msg; + struct pmic_glink_hdr *hdr = data; struct altmode_dev *amdev = priv; struct altmode_client *amclient; + u8 payload_len; u8 port_index; pr_debug("len: %zu owner: %u type: %u opcode %04x\n", len, hdr->owner, @@ -517,17 +539,28 @@ static int altmode_callback(void *priv, void *data, size_t len) */ op = GET_OP(hdr->opcode); svid = GET_SVID(hdr->opcode); - port_index = notify_msg->payload[0]; - - mutex_lock(&amdev->client_lock); - amclient = idr_find(&amdev->client_idr, IDR_KEY_GEN(svid, port_index)); - mutex_unlock(&amdev->client_lock); switch (op) { case USBC_CMD_WRITE_REQ: complete(&amdev->response_received); break; case USBC_NOTIFY_IND: + payload_len = NOTIFY_PAYLOAD_SIZE; + if (len < sizeof(*notify_msg)) + payload_len = len - sizeof(*hdr); + + /* payload should at least contain 9 bytes */ + if (payload_len < MIN_PAYLOAD_SIZE) + return -EINVAL; + + notify_msg = data; + port_index = notify_msg->payload[0]; + + mutex_lock(&amdev->client_lock); + amclient = idr_find(&amdev->client_idr, IDR_KEY_GEN(svid, + port_index)); + mutex_unlock(&amdev->client_lock); + if (!amclient) { pr_debug("No client associated with SVID %#x port %u\n", svid, port_index); @@ -537,10 +570,10 @@ static int altmode_callback(void *priv, void *data, size_t len) return 0; } - pr_debug("Payload: %*ph\n", NOTIFY_PAYLOAD_SIZE, - notify_msg->payload); - amclient->data.callback(amclient->data.priv, - notify_msg->payload, len); + pr_debug("Payload: %*ph\n", payload_len, notify_msg->payload); + cancel_work_sync(&amclient->client_cb_work); + memcpy(&amclient->msg, notify_msg->payload, payload_len); + schedule_work(&amclient->client_cb_work); break; default: break; From 1c675f15c0145c2751f091fa649362e6c6f1d0da Mon Sep 17 00:00:00 2001 From: Guru Das Srinagesh Date: Wed, 22 Jul 2020 16:54:12 -0700 Subject: [PATCH 6/6] soc: altmode: Add correct notify message length check Add a check to ensure that the remote subsystem sends the full and expected length of the notification message. Change-Id: I78d2c708c924cb864b7636bda81f290e310beb54 Signed-off-by: Guru Das Srinagesh --- drivers/soc/qcom/altmode-glink.c | 19 +++++++++---------- 1 file changed, 9 insertions(+), 10 deletions(-) diff --git a/drivers/soc/qcom/altmode-glink.c b/drivers/soc/qcom/altmode-glink.c index 9dabf6a7d3e5..ecf1e7a9af09 100644 --- a/drivers/soc/qcom/altmode-glink.c +++ b/drivers/soc/qcom/altmode-glink.c @@ -19,7 +19,6 @@ #define MSG_OWNER_USBC_PAN 32780 #define MSG_TYPE_REQ_RESP 1 -#define MIN_PAYLOAD_SIZE 9 #define NOTIFY_PAYLOAD_SIZE 16 #define USBC_WRITE_BUFFER_SIZE 8 @@ -526,7 +525,6 @@ static int altmode_callback(void *priv, void *data, size_t len) struct pmic_glink_hdr *hdr = data; struct altmode_dev *amdev = priv; struct altmode_client *amclient; - u8 payload_len; u8 port_index; pr_debug("len: %zu owner: %u type: %u opcode %04x\n", len, hdr->owner, @@ -545,13 +543,11 @@ static int altmode_callback(void *priv, void *data, size_t len) complete(&amdev->response_received); break; case USBC_NOTIFY_IND: - payload_len = NOTIFY_PAYLOAD_SIZE; - if (len < sizeof(*notify_msg)) - payload_len = len - sizeof(*hdr); - - /* payload should at least contain 9 bytes */ - if (payload_len < MIN_PAYLOAD_SIZE) + if (len != sizeof(*notify_msg)) { + pr_debug("Expected length %u, got: %zu\n", + sizeof(*notify_msg), len); return -EINVAL; + } notify_msg = data; port_index = notify_msg->payload[0]; @@ -570,9 +566,12 @@ static int altmode_callback(void *priv, void *data, size_t len) return 0; } - pr_debug("Payload: %*ph\n", payload_len, notify_msg->payload); + pr_debug("Payload: %*ph\n", NOTIFY_PAYLOAD_SIZE, + notify_msg->payload); + cancel_work_sync(&amclient->client_cb_work); - memcpy(&amclient->msg, notify_msg->payload, payload_len); + memcpy(&amclient->msg, notify_msg->payload, + sizeof(amclient->msg)); schedule_work(&amclient->client_cb_work); break; default: