| 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, 7 Aug 2013 11:35:58 -0700
From: Julius Werner <jwerner@...omium.org>
To: Vivek Gautam <gautam.vivek@...sung.com>
Cc: "linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>,
"linux-samsung-soc@...r.kernel.org"
<linux-samsung-soc@...r.kernel.org>,
LKML <linux-kernel@...r.kernel.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
Alan Stern <stern@...land.harvard.edu>,
Felipe Balbi <balbi@...com>, kishon@...com,
Julius Werner <jwerner@...omium.org>
Subject: Re: [PATCH] usb: phy: Cleanup error code in **_usb_get_phy_**() APIs
> @@ -94,11 +94,11 @@ static int devm_usb_phy_match(struct device *dev, void *res, void *match_data)
> */
> struct usb_phy *devm_usb_get_phy(struct device *dev, enum usb_phy_type type)
> {
> - struct usb_phy **ptr, *phy;
> + struct usb_phy *phy = ERR_PTR(-ENOMEM), **ptr;
This looks a little roundabout, don't you think? Why don't you just
directly have 'return ERR_PTR(-ENOMEM)' down there where you put 'goto
err0'?
>
> ptr = devres_alloc(devm_usb_phy_release, sizeof(*ptr), GFP_KERNEL);
> if (!ptr)
> - return NULL;
> + goto err0;
>
> phy = usb_get_phy(type);
> if (!IS_ERR(phy)) {
> @@ -107,6 +107,7 @@ struct usb_phy *devm_usb_get_phy(struct device *dev, enum usb_phy_type type)
> } else
> devres_free(ptr);
>
> +err0:
> return phy;
> }
> EXPORT_SYMBOL_GPL(devm_usb_get_phy);
> struct usb_phy *devm_usb_get_phy_dev(struct device *dev, u8 index)
> {
> - struct usb_phy **ptr, *phy;
> + struct usb_phy *phy = ERR_PTR(-ENOMEM), **ptr;
Same here
>
> ptr = devres_alloc(devm_usb_phy_release, sizeof(*ptr), GFP_KERNEL);
> if (!ptr)
> - return NULL;
> + goto err0;
>
> phy = usb_get_phy_dev(dev, index);
> if (!IS_ERR(phy)) {
> @@ -267,6 +268,7 @@ struct usb_phy *devm_usb_get_phy_dev(struct device *dev, u8 index)
> } else
> devres_free(ptr);
>
> +err0:
> return phy;
> }
> EXPORT_SYMBOL_GPL(devm_usb_get_phy_dev);
> @@ -142,7 +142,7 @@ extern void usb_remove_phy(struct usb_phy *);
> /* helpers for direct access thru low-level io interface */
> static inline int usb_phy_io_read(struct usb_phy *x, u32 reg)
> {
> - if (x->io_ops && x->io_ops->read)
> + if (!IS_ERR(x) && x->io_ops && x->io_ops->read)
I liked the ones where we had IS_ERR_OR_NULL() here (and in all the
ones below)... you sometimes have to handle PHYs in
platform-independent code where you don't want to worry about if this
platform actually has a PHY driver there or not. Any reason you
changed that?
> return x->io_ops->read(x, reg);
>
> return -EINVAL;
> @@ -150,7 +150,7 @@ static inline int usb_phy_io_read(struct usb_phy *x, u32 reg)
>
> static inline int usb_phy_io_write(struct usb_phy *x, u32 val, u32 reg)
> {
> - if (x->io_ops && x->io_ops->write)
> + if (!IS_ERR(x) && x->io_ops && x->io_ops->write)
> return x->io_ops->write(x, val, reg);
>
> return -EINVAL;
> @@ -159,7 +159,7 @@ static inline int usb_phy_io_write(struct usb_phy *x, u32 val, u32 reg)
> static inline int
> usb_phy_init(struct usb_phy *x)
> {
> - if (x->init)
> + if (!IS_ERR(x) && x->init)
> return x->init(x);
>
> return 0;
> @@ -168,26 +168,27 @@ usb_phy_init(struct usb_phy *x)
> static inline void
> usb_phy_shutdown(struct usb_phy *x)
> {
> - if (x->shutdown)
> + if (!IS_ERR(x) && x->shutdown)
> x->shutdown(x);
> }
>
> static inline int
> usb_phy_vbus_on(struct usb_phy *x)
> {
> - if (!x->set_vbus)
> - return 0;
> + if (!IS_ERR(x) && x->set_vbus)
> + return x->set_vbus(x, true);
>
> - return x->set_vbus(x, true);
> + return 0;
> }
>
> static inline int
> usb_phy_vbus_off(struct usb_phy *x)
> {
> - if (!x->set_vbus)
> - return 0;
> + if (!IS_ERR(x) && x->set_vbus)
> + return x->set_vbus(x, false);
> +
> + return 0;
>
> - return x->set_vbus(x, false);
> }
>
> /* for usb host and peripheral controller drivers */
> @@ -249,8 +250,9 @@ static inline int usb_bind_phy(const char *dev_name, u8 index,
> static inline int
> usb_phy_set_power(struct usb_phy *x, unsigned mA)
> {
> - if (x && x->set_power)
> + if (!IS_ERR(x) && x->set_power)
> return x->set_power(x, mA);
> +
> return 0;
> }
>
> @@ -258,28 +260,28 @@ usb_phy_set_power(struct usb_phy *x, unsigned mA)
> static inline int
> usb_phy_set_suspend(struct usb_phy *x, int suspend)
> {
> - if (x->set_suspend != NULL)
> + if (!IS_ERR(x) && x->set_suspend != NULL)
> return x->set_suspend(x, suspend);
> - else
> - return 0;
> +
> + return 0;
> }
>
> static inline int
> usb_phy_notify_connect(struct usb_phy *x, enum usb_device_speed speed)
> {
> - if (x->notify_connect)
> + if (!IS_ERR(x) && x->notify_connect)
> return x->notify_connect(x, speed);
> - else
> - return 0;
> +
> + return 0;
> }
>
> static inline int
> usb_phy_notify_disconnect(struct usb_phy *x, enum usb_device_speed speed)
> {
> - if (x->notify_disconnect)
> + if (!IS_ERR(x) && x->notify_disconnect)
> return x->notify_disconnect(x, speed);
> - else
> - return 0;
> +
> + return 0;
> }
Otherwise looks good to me.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Powered by blists - more mailing lists