Linux-USB Archive on lore.kernel.org
 help / color / Atom feed
From: Jun Li <jun.li@nxp.com>
To: Dan Carpenter <dan.carpenter@oracle.com>
Cc: Pengutronix Kernel Team <kernel@pengutronix.de>,
	dl-linux-imx <linux-imx@nxp.com>,
	"linux-usb@vger.kernel.org" <linux-usb@vger.kernel.org>
Subject: RE: [bug report] usb: chipidea: imx: enable vbus and id wakeup only for OTG events
Date: Tue, 8 Oct 2019 02:55:48 +0000
Message-ID: <VE1PR04MB65282B69FD17EC1583C52672899A0@VE1PR04MB6528.eurprd04.prod.outlook.com> (raw)
In-Reply-To: <20191002112954.GA1890@mwanda>

Hi Dan,
> -----Original Message-----
> From: Dan Carpenter <dan.carpenter@oracle.com>
> Sent: 2019年10月2日 19:30
> To: Jun Li <jun.li@nxp.com>
> Cc: Pengutronix Kernel Team <kernel@pengutronix.de>; dl-linux-imx
> <linux-imx@nxp.com>; linux-usb@vger.kernel.org
> Subject: [bug report] usb: chipidea: imx: enable vbus and id wakeup only for OTG events
> 
> Hello Li Jun,
> 
> The patch 15b80f7c3a7f: "usb: chipidea: imx: enable vbus and id wakeup only for OTG
> events" from Sep 9, 2019, leads to the following static checker warning:
> 
> 	drivers/usb/chipidea/ci_hdrc_imx.c:438 ci_hdrc_imx_probe()
> 	warn: 'data->usbmisc_data' can also be NULL

Thanks for the reporting.

> 
> drivers/usb/chipidea/ci_hdrc_imx.c
>    317          data = devm_kzalloc(&pdev->dev, sizeof(*data), GFP_KERNEL);
>    318          if (!data)
>    319                  return -ENOMEM;
>    320
>    321          data->plat_data = imx_platform_flag;
>    322          pdata.flags |= imx_platform_flag->flags;
>    323          platform_set_drvdata(pdev, data);
>    324          data->usbmisc_data = usbmisc_get_init_data(dev);
>                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> This can return NULL or error pointer.
> 
>    325          if (IS_ERR(data->usbmisc_data))
>    326                  return PTR_ERR(data->usbmisc_data);
>    327
>    328          if ((of_usb_get_phy_mode(dev->of_node) ==
> USBPHY_INTERFACE_MODE_HSIC)
>    329                  && data->usbmisc_data) {
>                            ^^^^^^^^^^^^^^^^^^ This code checks for NULL.
> 
>    330                  pdata.flags |= CI_HDRC_IMX_IS_HSIC;
>    331                  data->usbmisc_data->hsic = 1;
>    332                  data->pinctrl = devm_pinctrl_get(dev);
>    333                  if (IS_ERR(data->pinctrl)) {
>    334                          dev_err(dev, "pinctrl get failed, err=%ld\n",
>    335                                          PTR_ERR(data->pinctrl));
>    336                          return PTR_ERR(data->pinctrl);
>    337                  }
>    338
>    339                  pinctrl_hsic_idle = pinctrl_lookup_state(data->pinctrl, "idle");
>    340                  if (IS_ERR(pinctrl_hsic_idle)) {
>    341                          dev_err(dev,
>    342                                  "pinctrl_hsic_idle lookup failed, err=%ld\n",
>    343                                          PTR_ERR(pinctrl_hsic_idle));
>    344                          return PTR_ERR(pinctrl_hsic_idle);
>    345                  }
>    346
>    347                  ret = pinctrl_select_state(data->pinctrl, pinctrl_hsic_idle);
>    348                  if (ret) {
>    349                          dev_err(dev, "hsic_idle select failed, err=%d\n", ret);
>    350                          return ret;
>    351                  }
>    352
>    353                  data->pinctrl_hsic_active = pinctrl_lookup_state(data->pinctrl,
>    354                                                                  "active");
>    355                  if (IS_ERR(data->pinctrl_hsic_active)) {
>    356                          dev_err(dev,
>    357                                  "pinctrl_hsic_active lookup failed, err=%ld\n",
>    358
> PTR_ERR(data->pinctrl_hsic_active));
>    359                          return PTR_ERR(data->pinctrl_hsic_active);
>    360                  }
>    361
>    362                  data->hsic_pad_regulator = devm_regulator_get(dev, "hsic");
>    363                  if (PTR_ERR(data->hsic_pad_regulator) ==
> -EPROBE_DEFER) {
>    364                          return -EPROBE_DEFER;
>    365                  } else if (PTR_ERR(data->hsic_pad_regulator) == -ENODEV)
> {
>    366                          /* no pad regualator is needed */
>    367                          data->hsic_pad_regulator = NULL;
>    368                  } else if (IS_ERR(data->hsic_pad_regulator)) {
>    369                          dev_err(dev, "Get HSIC pad regulator error: %ld\n",
>    370
> PTR_ERR(data->hsic_pad_regulator));
>    371                          return PTR_ERR(data->hsic_pad_regulator);
>    372                  }
>    373
>    374                  if (data->hsic_pad_regulator) {
>    375                          ret = regulator_enable(data->hsic_pad_regulator);
>    376                          if (ret) {
>    377                                  dev_err(dev,
>    378                                          "Failed to enable HSIC pad
> regulator\n");
>    379                                  return ret;
>    380                          }
>    381                  }
>    382          }
>    383
>    384          if (pdata.flags & CI_HDRC_PMQOS)
>    385                  pm_qos_add_request(&data->pm_qos_req,
>    386                          PM_QOS_CPU_DMA_LATENCY, 0);
>    387
>    388          ret = imx_get_clks(dev);
>    389          if (ret)
>    390                  goto disable_hsic_regulator;
>    391
>    392          ret = imx_prepare_enable_clks(dev);
>    393          if (ret)
>    394                  goto disable_hsic_regulator;
>    395
>    396          data->phy = devm_usb_get_phy_by_phandle(dev, "fsl,usbphy", 0);
>    397          if (IS_ERR(data->phy)) {
>    398                  ret = PTR_ERR(data->phy);
>    399                  /* Return -EINVAL if no usbphy is available */
>    400                  if (ret == -ENODEV)
>    401                          data->phy = NULL;
>    402                  else
>    403                          goto err_clk;
>    404          }
>    405
>    406          pdata.usb_phy = data->phy;
>    407
>    408          if ((of_device_is_compatible(np, "fsl,imx53-usb") ||
>    409               of_device_is_compatible(np, "fsl,imx51-usb")) && pdata.usb_phy
> &&
>    410              of_usb_get_phy_mode(np) ==
> USBPHY_INTERFACE_MODE_ULPI) {
>    411                  pdata.flags |= CI_HDRC_OVERRIDE_PHY_CONTROL;
>    412                  data->override_phy_control = true;
>    413                  usb_phy_init(pdata.usb_phy);
>    414          }
>    415
>    416          if (pdata.flags & CI_HDRC_SUPPORTS_RUNTIME_PM)
>    417                  data->supports_runtime_pm = true;
>    418
>    419          ret = imx_usbmisc_init(data->usbmisc_data);
>    420          if (ret) {
>    421                  dev_err(dev, "usbmisc init failed, ret=%d\n", ret);
>    422                  goto err_clk;
>    423          }
>    424
>    425          data->ci_pdev = ci_hdrc_add_device(dev,
>    426                                  pdev->resource, pdev->num_resources,
>    427                                  &pdata);
>    428          if (IS_ERR(data->ci_pdev)) {
>    429                  ret = PTR_ERR(data->ci_pdev);
>    430                  if (ret != -EPROBE_DEFER)
>    431                          dev_err(dev, "ci_hdrc_add_device failed, err=%d\n",
>    432                                          ret);
>    433                  goto err_clk;
>    434          }
>    435
>    436          if (!IS_ERR(pdata.id_extcon.edev) ||
>    437              of_property_read_bool(np, "usb-role-switch"))
>    438                  data->usbmisc_data->ext_id = 1;
>                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
> 
>    439
>    440          if (!IS_ERR(pdata.vbus_extcon.edev) ||
>    441              of_property_read_bool(np, "usb-role-switch"))
>    442                  data->usbmisc_data->ext_vbus = 1;
>                         ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ These should probably
> check for NULL?  This driver seems to check pretty consistently.

Yes, I will submit a patch to fix this.

Li Jun
> 
>    443
>    444          ret = imx_usbmisc_init_post(data->usbmisc_data);
> 
> regards,
> dan carpenter

      reply index

Thread overview: 2+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-10-02 11:29 Dan Carpenter
2019-10-08  2:55 ` Jun Li [this message]

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=VE1PR04MB65282B69FD17EC1583C52672899A0@VE1PR04MB6528.eurprd04.prod.outlook.com \
    --to=jun.li@nxp.com \
    --cc=dan.carpenter@oracle.com \
    --cc=kernel@pengutronix.de \
    --cc=linux-imx@nxp.com \
    --cc=linux-usb@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Linux-USB Archive on lore.kernel.org

Archives are clonable:
	git clone --mirror https://lore.kernel.org/linux-usb/0 linux-usb/git/0.git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V2 linux-usb linux-usb/ https://lore.kernel.org/linux-usb \
		linux-usb@vger.kernel.org
	public-inbox-index linux-usb

Example config snippet for mirrors

Newsgroup available over NNTP:
	nntp://nntp.lore.kernel.org/org.kernel.vger.linux-usb


AGPL code for this site: git clone https://public-inbox.org/public-inbox.git