| 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: <c5c93284116d4fe4800778dd49aac0ec@realtek.com>
Date: Tue, 20 Jun 2023 08:58:56 +0000
From: Stanley Chang[昌育德]
<stanley_chang@...ltek.com>
To: Krzysztof Kozlowski <krzysztof.kozlowski@...aro.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>
CC: Vinod Koul <vkoul@...nel.org>,
Kishon Vijay Abraham I <kishon@...nel.org>,
Rob Herring <robh+dt@...nel.org>,
Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
Conor Dooley <conor+dt@...nel.org>,
Alan Stern <stern@...land.harvard.edu>,
Flavio Suligoi <f.suligoi@...m.it>,
Douglas Anderson <dianders@...omium.org>,
Bagas Sanjaya <bagasdotme@...il.com>,
Matthias Kaehlcke <mka@...omium.org>,
Ray Chi <raychi@...gle.com>,
"linux-phy@...ts.infradead.org" <linux-phy@...ts.infradead.org>,
"devicetree@...r.kernel.org" <devicetree@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-usb@...r.kernel.org" <linux-usb@...r.kernel.org>
Subject: RE: [PATCH v4 2/5] phy: realtek: usb: Add driver for the Realtek SoC USB 2.0 PHY
Hi Krzysztof,
> > + addr = phy_data->addr;
> > + data = phy_data->data;
> > + dc_disconnect_mask = phy_cfg->dc_disconnect_mask;
> > +
> > + if (update)
> > + data =
> __updated_dc_disconnect_level_page0_0xe4(phy_cfg, phy_parameter, data);
> > + else
> > + data = (data & ~(dc_disconnect_mask << offset)) |
> > + (DEFAULT_DC_DISCONNECTION_VALUE <<
> offset);
> > +
> > + if (rtk_phy_write(phy_reg, addr, data))
> > + dev_err(rtk_phy->dev,
> > + "[%s:%d] Error page1 addr=0x%x value=0x%x\n",
> > + __func__, __LINE__,
> > + addr, data);
>
> Is addr a kernel address or any memory (not SFR) address? If so, you cannot
> print it.
It is not memory address.
> > +
> > + if (rtk_phy_write(phy_reg, addr, data))
> > + dev_err(rtk_phy->dev,
> > + "[%s:%d] Error page1 addr=0x%x value=0x%x\n",
> > + __func__, __LINE__,
> > + addr, data);
>
> Ditto and in all other places.
It is not memory address.
> > +static u8 __update_dc_driving_page0_0xe4(struct phy_cfg *phy_cfg,
> > + struct phy_parameter
> > +*phy_parameter, u8 data) {
> > + s32 driving_compensate = phy_parameter->driving_compensate;
> > + s32 dc_driving_mask = phy_cfg->dc_driving_mask;
> > + s32 __val;
> > + u8 val;
>
> Two variables with the same name. No, it is not readable code.
Okay. I will revise it.
> > +static void rtk_phy_toggle(struct usb_phy *usb2_phy, bool connect,
> > +int port) {
> > + int index = port;
> > + struct rtk_phy *rtk_phy = NULL;
> > +
> > + rtk_phy = dev_get_drvdata(usb2_phy->dev);
> > +
> > + if (index > rtk_phy->num_phy) {
> > + pr_err("%s %d ERROR! port=%d > num_phy=%d\n",
>
> dev_err
I revised it.
> > + __func__, __LINE__, index, rtk_phy->num_phy);
>
> all these func and LINE point to poor code quality and poor debugging
> practices. These are added dugin development, not for production code,
> because error message should be obvious. Your usage of pr_err, func, LINE and
> some unprecise messages suggests this is not ready.
>
> Fix all your error messages to be meaningful.
I will review all error messages.
Thanks.
> > +static const struct file_operations rtk_usb2_set_parameter_fops = {
> > + .open = rtk_usb2_set_parameter_open,
> > + .write = rtk_usb2_set_parameter_write,
>
> NAK. You just created user interface via debugfs. You cannot. Reading for some
> debug is okay, but configuring device via undocumented debugfs is a source of
> troubles.
>
> Drop all writes to debugfs.
I will remove this.
>
> > +
> > +static int parse_phy_data(struct rtk_phy *rtk_phy) {
> > + struct device *dev = rtk_phy->dev;
> > + struct device_node *node;
>
> By convention:
> s/node/np/
Okay.
> > + struct phy_cfg *phy_cfg;
> > + struct phy_parameter *phy_parameter;
> > + int ret = 0;
> > + int index;
> > +
> > + node = dev->of_node;
>
> Keep it in variable definition.
Okay.
> > +
> > +static int rtk_usb2phy_probe(struct platform_device *pdev) {
> > + struct rtk_phy *rtk_phy;
> > + struct device *dev = &pdev->dev;
> > + struct device_node *node;
> > + struct phy *generic_phy;
> > + struct phy_provider *phy_provider;
> > + const struct phy_cfg *phy_cfg;
> > + int ret = 0;
> > +
> > + phy_cfg = of_device_get_match_data(dev);
> > + if (!phy_cfg) {
> > + dev_err(dev, "phy config are not assigned!\n");
> > + return -EINVAL;
> > + }
> > +
> > + rtk_phy = devm_kzalloc(dev, sizeof(*rtk_phy), GFP_KERNEL);
> > + if (!rtk_phy)
> > + return -ENOMEM;
> > +
> > + rtk_phy->dev = &pdev->dev;
> > + rtk_phy->phy.dev = rtk_phy->dev;
> > + rtk_phy->phy.label = "rtk-usb2phy";
> > + rtk_phy->phy.notify_port_status = rtk_phy_notify_port_status;
> > +
> > + rtk_phy->phy_cfg = devm_kzalloc(dev, sizeof(*phy_cfg),
> > + GFP_KERNEL);
> > +
> > + memcpy(rtk_phy->phy_cfg, phy_cfg, sizeof(*phy_cfg));
> > +
> > + node = dev->of_node;
>
> Drop it. Useless assignment.
Okay.
> > +
> > + if (of_device_is_compatible(node,
> > + "realtek,rtd1395-usb2phy-2port"))
>
> No, customize variant with driver_data. Don't embed compatibles in the code.
I will use the compatible data to match this case.
>
> > + rtk_phy->num_phy = 2;
> > + else
> > + rtk_phy->num_phy = 1;
> > +
> > + ret = parse_phy_data(rtk_phy);
> > + if (ret)
> > + goto err;
> > +
> > + platform_set_drvdata(pdev, rtk_phy);
> > +
> > + generic_phy = devm_phy_create(rtk_phy->dev, NULL, &ops);
> > + if (IS_ERR(generic_phy))
> > + return PTR_ERR(generic_phy);
> > +
> > + phy_set_drvdata(generic_phy, rtk_phy);
> > +
> > + phy_provider = devm_of_phy_provider_register(rtk_phy->dev,
> > +
> of_phy_simple_xlate);
> > + if (IS_ERR(phy_provider))
> > + return PTR_ERR(phy_provider);
> > +
> > + ret = usb_add_phy_dev(&rtk_phy->phy);
> > + if (ret)
> > + goto err;
> > +
> > + create_debug_files(rtk_phy);
> > +
> > +err:
> > + dev_dbg(dev, "Probe RTK USB 2.0 PHY (ret=%d)\n", ret);
>
> NAK. I made it pretty clear last time.
>
>
> This is a friendly reminder during the review process.
>
> It seems my previous comments were not fully addressed. Maybe my feedback
> got lost between the quotes, maybe you just forgot to apply it.
> Please go back to the previous discussion and either implement all requested
> changes or keep discussing them.
>
> Thank you.
Sorry. I left out this print, I will delete it.
Thank you.
> > +
> > + return ret;
> > +}
> > +
> > +static void rtk_usb2phy_remove(struct platform_device *pdev) {
> > + struct rtk_phy *rtk_phy = platform_get_drvdata(pdev);
> > +
> > + remove_debug_files(rtk_phy);
> > +
> > + usb_remove_phy(&rtk_phy->phy);
> > +}
>
> ...
>
> > +
> > +static const struct of_device_id usbphy_rtk_dt_match[] = {
> > + { .compatible = "realtek,rtd1295-usb2phy", .data =
> &rtd1295_phy_cfg },
> > + { .compatible = "realtek,rtd1312c-usb2phy", .data =
> &rtd1312c_phy_cfg },
> > + { .compatible = "realtek,rtd1315e-usb2phy", .data =
> &rtd1315e_phy_cfg },
> > + { .compatible = "realtek,rtd1319-usb2phy", .data =
> &rtd1319_phy_cfg },
> > + { .compatible = "realtek,rtd1319d-usb2phy", .data =
> &rtd1319d_phy_cfg },
> > + { .compatible = "realtek,rtd1395-usb2phy", .data =
> &rtd1395_phy_cfg },
> > + { .compatible = "realtek,rtd1395-usb2phy-2port", .data =
> &rtd1395_phy_cfg },
> > + { .compatible = "realtek,rtd1619-usb2phy", .data =
> &rtd1619_phy_cfg },
> > + { .compatible = "realtek,rtd1619b-usb2phy", .data =
> &rtd1619b_phy_cfg },
> > + { .compatible = "realtek,usb2phy", .data = &rtk_phy_cfg },
>
> This is now even more suprising. Drop "realtek,usb2phy"
I will remove it.
> > + {},
> > +};
> > +MODULE_DEVICE_TABLE(of, usbphy_rtk_dt_match);
> > +
> > +static struct platform_driver rtk_usb2phy_driver = {
> > + .probe = rtk_usb2phy_probe,
> > + .remove_new = rtk_usb2phy_remove,
> > + .driver = {
> > + .name = "rtk-usb2phy",
> > + .owner = THIS_MODULE,
>
> ??? Didn't you base your driver on some really, really ancient code (like 5 years
> old)? If so, please don't.
Thank you.
I will remove it.
> Run coccicenelle/coccicheck, smatch and sparse, to avoid common mistakes.
>
I will run these tools.
Thanks,
Stanley
Powered by blists - more mailing lists