| 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
| ||
|
Message-ID: <f7becefe-a297-8cd5-b490-05b19594d3ee@roeck-us.net>
Date: Sun, 25 Feb 2018 13:38:12 -0800
From: Guenter Roeck <linux@...ck-us.net>
To: Hans de Goede <hdegoede@...hat.com>,
Darren Hart <dvhart@...radead.org>,
Andy Shevchenko <andy@...radead.org>,
MyungJoo Ham <myungjoo.ham@...sung.com>,
Chanwoo Choi <cw00.choi@...sung.com>,
Mathias Nyman <mathias.nyman@...el.com>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Heikki Krogerus <heikki.krogerus@...ux.intel.com>
Cc: platform-driver-x86@...r.kernel.org, linux-kernel@...r.kernel.org,
linux-usb@...r.kernel.org
Subject: Re: [PATCH v2 02/12] usb: typec: Start using ERR_PTR
On 02/25/2018 07:25 AM, Hans de Goede wrote:
> From: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
>
> In order to allow the USB Type-C Class driver take care of
> things like muxes and other possible dependencies for the
> port drivers, returning ERR_PTR instead of NULL from the
> registration functions in case of failure.
>
> The reason for taking over control of the muxes for example
> is because handling them in the port drivers would be just
> boilerplate.
>
> Signed-off-by: Heikki Krogerus <heikki.krogerus@...ux.intel.com>
> Reviewed-by: Hans de Goede <hdegoede@...hat.com>
> Signed-off-by: Hans de Goede <hdegoede@...hat.com>
Reviewed-by: Guenter Roeck <linux@...ck-us.net>
> ---
> Changes in v2:
> -Add IS_ERR_OR_NULL() checks to the unregister functions
> ---
> drivers/usb/typec/tcpm.c | 16 +++++++++-------
> drivers/usb/typec/tps6598x.c | 15 ++++++++-------
> drivers/usb/typec/typec.c | 44 +++++++++++++++++++++----------------------
> drivers/usb/typec/ucsi/ucsi.c | 31 ++++++++++++++++++------------
> 4 files changed, 58 insertions(+), 48 deletions(-)
>
> diff --git a/drivers/usb/typec/tcpm.c b/drivers/usb/typec/tcpm.c
> index f4d563ee7690..7cd28b700a7f 100644
> --- a/drivers/usb/typec/tcpm.c
> +++ b/drivers/usb/typec/tcpm.c
> @@ -1044,7 +1044,7 @@ static int tcpm_pd_svdm(struct tcpm_port *port, const __le32 *payload, int cnt,
> break;
> case CMDT_RSP_ACK:
> /* silently drop message if we are not connected */
> - if (!port->partner)
> + if (IS_ERR_OR_NULL(port->partner))
> break;
>
> switch (cmd) {
> @@ -3743,8 +3743,8 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
> port->port_type = tcpc->config->type;
>
> port->typec_port = typec_register_port(port->dev, &port->typec_caps);
> - if (!port->typec_port) {
> - err = -ENOMEM;
> + if (IS_ERR(port->typec_port)) {
> + err = PTR_ERR(port->typec_port);
> goto out_destroy_wq;
> }
>
> @@ -3753,15 +3753,17 @@ struct tcpm_port *tcpm_register_port(struct device *dev, struct tcpc_dev *tcpc)
>
> i = 0;
> while (paltmode->svid && i < ARRAY_SIZE(port->port_altmode)) {
> - port->port_altmode[i] =
> - typec_port_register_altmode(port->typec_port,
> - paltmode);
> - if (!port->port_altmode[i]) {
> + struct typec_altmode *alt;
> +
> + alt = typec_port_register_altmode(port->typec_port,
> + paltmode);
> + if (IS_ERR(alt)) {
> tcpm_log(port,
> "%s: failed to register port alternate mode 0x%x",
> dev_name(dev), paltmode->svid);
> break;
> }
> + port->port_altmode[i] = alt;
> i++;
> paltmode++;
> }
> diff --git a/drivers/usb/typec/tps6598x.c b/drivers/usb/typec/tps6598x.c
> index 2719f5d382f7..37a15c14a6c6 100644
> --- a/drivers/usb/typec/tps6598x.c
> +++ b/drivers/usb/typec/tps6598x.c
> @@ -158,15 +158,15 @@ static int tps6598x_connect(struct tps6598x *tps, u32 status)
> desc.identity = &tps->partner_identity;
> }
>
> - tps->partner = typec_register_partner(tps->port, &desc);
> - if (!tps->partner)
> - return -ENODEV;
> -
> typec_set_pwr_opmode(tps->port, mode);
> typec_set_pwr_role(tps->port, TPS_STATUS_PORTROLE(status));
> typec_set_vconn_role(tps->port, TPS_STATUS_VCONN(status));
> typec_set_data_role(tps->port, TPS_STATUS_DATAROLE(status));
>
> + tps->partner = typec_register_partner(tps->port, &desc);
> + if (IS_ERR(tps->partner))
> + return PTR_ERR(tps->partner);
> +
> if (desc.identity)
> typec_partner_set_identity(tps->partner);
>
> @@ -175,7 +175,8 @@ static int tps6598x_connect(struct tps6598x *tps, u32 status)
>
> static void tps6598x_disconnect(struct tps6598x *tps, u32 status)
> {
> - typec_unregister_partner(tps->partner);
> + if (!IS_ERR(tps->partner))
> + typec_unregister_partner(tps->partner);
> tps->partner = NULL;
> typec_set_pwr_opmode(tps->port, TYPEC_PWR_MODE_USB);
> typec_set_pwr_role(tps->port, TPS_STATUS_PORTROLE(status));
> @@ -418,8 +419,8 @@ static int tps6598x_probe(struct i2c_client *client)
> tps->typec_cap.prefer_role = TYPEC_NO_PREFERRED_ROLE;
>
> tps->port = typec_register_port(&client->dev, &tps->typec_cap);
> - if (!tps->port)
> - return -ENODEV;
> + if (IS_ERR(tps->port))
> + return PTR_ERR(tps->port);
>
> if (status & TPS_STATUS_PLUG_PRESENT) {
> ret = tps6598x_connect(tps, status);
> diff --git a/drivers/usb/typec/typec.c b/drivers/usb/typec/typec.c
> index 735726ced602..dc28ad868d93 100644
> --- a/drivers/usb/typec/typec.c
> +++ b/drivers/usb/typec/typec.c
> @@ -386,7 +386,7 @@ typec_register_altmode(struct device *parent,
>
> alt = kzalloc(sizeof(*alt), GFP_KERNEL);
> if (!alt)
> - return NULL;
> + return ERR_PTR(-ENOMEM);
>
> alt->svid = desc->svid;
> alt->n_modes = desc->n_modes;
> @@ -402,7 +402,7 @@ typec_register_altmode(struct device *parent,
> dev_err(parent, "failed to register alternate mode (%d)\n",
> ret);
> put_device(&alt->dev);
> - return NULL;
> + return ERR_PTR(ret);
> }
>
> return alt;
> @@ -417,7 +417,7 @@ typec_register_altmode(struct device *parent,
> */
> void typec_unregister_altmode(struct typec_altmode *alt)
> {
> - if (alt)
> + if (!IS_ERR_OR_NULL(alt))
> device_unregister(&alt->dev);
> }
> EXPORT_SYMBOL_GPL(typec_unregister_altmode);
> @@ -509,7 +509,7 @@ EXPORT_SYMBOL_GPL(typec_partner_register_altmode);
> *
> * Registers a device for USB Type-C Partner described in @desc.
> *
> - * Returns handle to the partner on success or NULL on failure.
> + * Returns handle to the partner on success or ERR_PTR on failure.
> */
> struct typec_partner *typec_register_partner(struct typec_port *port,
> struct typec_partner_desc *desc)
> @@ -519,7 +519,7 @@ struct typec_partner *typec_register_partner(struct typec_port *port,
>
> partner = kzalloc(sizeof(*partner), GFP_KERNEL);
> if (!partner)
> - return NULL;
> + return ERR_PTR(-ENOMEM);
>
> partner->usb_pd = desc->usb_pd;
> partner->accessory = desc->accessory;
> @@ -542,7 +542,7 @@ struct typec_partner *typec_register_partner(struct typec_port *port,
> if (ret) {
> dev_err(&port->dev, "failed to register partner (%d)\n", ret);
> put_device(&partner->dev);
> - return NULL;
> + return ERR_PTR(ret);
> }
>
> return partner;
> @@ -557,7 +557,7 @@ EXPORT_SYMBOL_GPL(typec_register_partner);
> */
> void typec_unregister_partner(struct typec_partner *partner)
> {
> - if (partner)
> + if (!IS_ERR_OR_NULL(partner))
> device_unregister(&partner->dev);
> }
> EXPORT_SYMBOL_GPL(typec_unregister_partner);
> @@ -587,7 +587,7 @@ static const struct device_type typec_plug_dev_type = {
> * the plug lists in response to Discover Modes command need to be listed in an
> * array in @desc.
> *
> - * Returns handle to the alternate mode on success or NULL on failure.
> + * Returns handle to the alternate mode on success or ERR_PTR on failure.
> */
> struct typec_altmode *
> typec_plug_register_altmode(struct typec_plug *plug,
> @@ -606,7 +606,7 @@ EXPORT_SYMBOL_GPL(typec_plug_register_altmode);
> * Cable Plug represents a plug with electronics in it that can response to USB
> * Power Delivery SOP Prime or SOP Double Prime packages.
> *
> - * Returns handle to the cable plug on success or NULL on failure.
> + * Returns handle to the cable plug on success or ERR_PTR on failure.
> */
> struct typec_plug *typec_register_plug(struct typec_cable *cable,
> struct typec_plug_desc *desc)
> @@ -617,7 +617,7 @@ struct typec_plug *typec_register_plug(struct typec_cable *cable,
>
> plug = kzalloc(sizeof(*plug), GFP_KERNEL);
> if (!plug)
> - return NULL;
> + return ERR_PTR(-ENOMEM);
>
> sprintf(name, "plug%d", desc->index);
>
> @@ -631,7 +631,7 @@ struct typec_plug *typec_register_plug(struct typec_cable *cable,
> if (ret) {
> dev_err(&cable->dev, "failed to register plug (%d)\n", ret);
> put_device(&plug->dev);
> - return NULL;
> + return ERR_PTR(ret);
> }
>
> return plug;
> @@ -646,7 +646,7 @@ EXPORT_SYMBOL_GPL(typec_register_plug);
> */
> void typec_unregister_plug(struct typec_plug *plug)
> {
> - if (plug)
> + if (!IS_ERR_OR_NULL(plug))
> device_unregister(&plug->dev);
> }
> EXPORT_SYMBOL_GPL(typec_unregister_plug);
> @@ -724,7 +724,7 @@ EXPORT_SYMBOL_GPL(typec_cable_set_identity);
> * Registers a device for USB Type-C Cable described in @desc. The cable will be
> * parent for the optional cable plug devises.
> *
> - * Returns handle to the cable on success or NULL on failure.
> + * Returns handle to the cable on success or ERR_PTR on failure.
> */
> struct typec_cable *typec_register_cable(struct typec_port *port,
> struct typec_cable_desc *desc)
> @@ -734,7 +734,7 @@ struct typec_cable *typec_register_cable(struct typec_port *port,
>
> cable = kzalloc(sizeof(*cable), GFP_KERNEL);
> if (!cable)
> - return NULL;
> + return ERR_PTR(-ENOMEM);
>
> cable->type = desc->type;
> cable->active = desc->active;
> @@ -757,7 +757,7 @@ struct typec_cable *typec_register_cable(struct typec_port *port,
> if (ret) {
> dev_err(&port->dev, "failed to register cable (%d)\n", ret);
> put_device(&cable->dev);
> - return NULL;
> + return ERR_PTR(ret);
> }
>
> return cable;
> @@ -772,7 +772,7 @@ EXPORT_SYMBOL_GPL(typec_register_cable);
> */
> void typec_unregister_cable(struct typec_cable *cable)
> {
> - if (cable)
> + if (!IS_ERR_OR_NULL(cable))
> device_unregister(&cable->dev);
> }
> EXPORT_SYMBOL_GPL(typec_unregister_cable);
> @@ -1256,7 +1256,7 @@ EXPORT_SYMBOL_GPL(typec_set_pwr_opmode);
> * This routine is used to register an alternate mode that @port is capable of
> * supporting.
> *
> - * Returns handle to the alternate mode on success or NULL on failure.
> + * Returns handle to the alternate mode on success or ERR_PTR on failure.
> */
> struct typec_altmode *
> typec_port_register_altmode(struct typec_port *port,
> @@ -1273,7 +1273,7 @@ EXPORT_SYMBOL_GPL(typec_port_register_altmode);
> *
> * Registers a device for USB Type-C Port described in @cap.
> *
> - * Returns handle to the port on success or NULL on failure.
> + * Returns handle to the port on success or ERR_PTR on failure.
> */
> struct typec_port *typec_register_port(struct device *parent,
> const struct typec_capability *cap)
> @@ -1285,12 +1285,12 @@ struct typec_port *typec_register_port(struct device *parent,
>
> port = kzalloc(sizeof(*port), GFP_KERNEL);
> if (!port)
> - return NULL;
> + return ERR_PTR(-ENOMEM);
>
> id = ida_simple_get(&typec_index_ida, 0, 0, GFP_KERNEL);
> if (id < 0) {
> kfree(port);
> - return NULL;
> + return ERR_PTR(id);
> }
>
> if (cap->type == TYPEC_PORT_DFP)
> @@ -1326,7 +1326,7 @@ struct typec_port *typec_register_port(struct device *parent,
> if (ret) {
> dev_err(parent, "failed to register port (%d)\n", ret);
> put_device(&port->dev);
> - return NULL;
> + return ERR_PTR(ret);
> }
>
> return port;
> @@ -1341,7 +1341,7 @@ EXPORT_SYMBOL_GPL(typec_register_port);
> */
> void typec_unregister_port(struct typec_port *port)
> {
> - if (port)
> + if (!IS_ERR_OR_NULL(port))
> device_unregister(&port->dev);
> }
> EXPORT_SYMBOL_GPL(typec_unregister_port);
> diff --git a/drivers/usb/typec/ucsi/ucsi.c b/drivers/usb/typec/ucsi/ucsi.c
> index 79046fe66426..69d544cfcd45 100644
> --- a/drivers/usb/typec/ucsi/ucsi.c
> +++ b/drivers/usb/typec/ucsi/ucsi.c
> @@ -260,38 +260,45 @@ static void ucsi_pwr_opmode_change(struct ucsi_connector *con)
>
> static int ucsi_register_partner(struct ucsi_connector *con)
> {
> - struct typec_partner_desc partner;
> + struct typec_partner_desc desc;
> + struct typec_partner *partner;
>
> if (con->partner)
> return 0;
>
> - memset(&partner, 0, sizeof(partner));
> + memset(&desc, 0, sizeof(desc));
>
> switch (con->status.partner_type) {
> case UCSI_CONSTAT_PARTNER_TYPE_DEBUG:
> - partner.accessory = TYPEC_ACCESSORY_DEBUG;
> + desc.accessory = TYPEC_ACCESSORY_DEBUG;
> break;
> case UCSI_CONSTAT_PARTNER_TYPE_AUDIO:
> - partner.accessory = TYPEC_ACCESSORY_AUDIO;
> + desc.accessory = TYPEC_ACCESSORY_AUDIO;
> break;
> default:
> break;
> }
>
> - partner.usb_pd = con->status.pwr_op_mode == UCSI_CONSTAT_PWR_OPMODE_PD;
> + desc.usb_pd = con->status.pwr_op_mode == UCSI_CONSTAT_PWR_OPMODE_PD;
>
> - con->partner = typec_register_partner(con->port, &partner);
> - if (!con->partner) {
> - dev_err(con->ucsi->dev, "con%d: failed to register partner\n",
> - con->num);
> - return -ENODEV;
> + partner = typec_register_partner(con->port, &desc);
> + if (IS_ERR(partner)) {
> + dev_err(con->ucsi->dev,
> + "con%d: failed to register partner (%ld)\n", con->num,
> + PTR_ERR(partner));
> + return PTR_ERR(partner);
> }
>
> + con->partner = partner;
> +
> return 0;
> }
>
> static void ucsi_unregister_partner(struct ucsi_connector *con)
> {
> + if (!con->partner)
> + return;
> +
> typec_unregister_partner(con->partner);
> con->partner = NULL;
> }
> @@ -606,8 +613,8 @@ static int ucsi_register_port(struct ucsi *ucsi, int index)
>
> /* Register the connector */
> con->port = typec_register_port(ucsi->dev, cap);
> - if (!con->port)
> - return -ENODEV;
> + if (IS_ERR(con->port))
> + return PTR_ERR(con->port);
>
> /* Get the status */
> UCSI_CMD_GET_CONNECTOR_STATUS(ctrl, con->num);
>
Powered by blists - more mailing lists