| 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: <0ac12a13a91d41f0ab3a58b435ccb17a@realtek.com>
Date: Thu, 8 Jun 2023 06:59:08 +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>,
Ray Chi <raychi@...gle.com>,
Michael Grzeschik <m.grzeschik@...gutronix.de>,
Flavio Suligoi <f.suligoi@...m.it>,
Matthias Kaehlcke <mka@...omium.org>,
Mathias Nyman <mathias.nyman@...ux.intel.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 v3 3/5] phy: realtek: usb: Add driver for the Realtek SoC USB 3.0 PHY
Hi Krzysztof,
> > +static void do_rtk_usb3_phy_toggle(struct rtk_usb_phy *rtk_phy, int i,
> > + bool isConnect)
> > +{
> > + struct reg_addr *regAddr;
> > + struct phy_data *phy_data;
> > + struct phy_parameter *phy_parameter;
> > + size_t index;
> > +
> > + regAddr = &((struct reg_addr *)rtk_phy->reg_addr)[i];
> > + phy_data = &((struct phy_data *)rtk_phy->phy_data)[i];
> > +
> > + if (!phy_data) {
> > + dev_err(rtk_phy->dev, "%s phy_data is NULL!\n",
> > + __func__);
>
> ???
Sorry, this check is redundant.
>
> > + return;
> > + }
> > +
> > + if (!phy_data->do_toggle)
> > + return;
> > +
> > + phy_parameter = phy_data->parameter;
> > +
> > + index = PHY_ADDR_MAP_ARRAY_INDEX(PHY_ADDR_0x09);
> > +
> > + if (index < phy_data->size) {
> > + u8 addr = (phy_parameter + index)->addr;
> > + u16 data = (phy_parameter + index)->data;
> > +
> > + if (addr == 0xFF) {
> > + addr = ARRAY_INDEX_MAP_PHY_ADDR(index);
> > + data = rtk_usb_phy_read(regAddr, addr);
> > + (phy_parameter + index)->addr = addr;
> > + (phy_parameter + index)->data = data;
> > + }
> > + mdelay(1);
> > + dev_info(rtk_phy->dev,
> > + "%s ########## to toggle PHY addr 0x09
> BIT(9)\n",
> > + __func__);
> > + rtk_usb_phy_write(regAddr, addr, data&(~BIT(9)));
> > + mdelay(1);
> > + rtk_usb_phy_write(regAddr, addr, data);
> > + }
> > + dev_info(rtk_phy->dev, "%s ########## PHY addr 0x1f = 0x%04x\n",
> > + __func__, rtk_usb_phy_read(regAddr,
> PHY_ADDR_0x1F));
>
> Please drop all simple debug success messages. Linux has already
> infrastructure for this.
>
Okay. I will change the print dev_info to dev_dbg about debug message.
> ...
>
> > + return 0;
> > +}
> > +
> > +static int rtk_usb_phy_init(struct phy *phy) {
> > + struct rtk_usb_phy *rtk_phy = phy_get_drvdata(phy);
> > + int ret = 0;
> > + int i;
> > + unsigned long phy_init_time = jiffies;
> > +
> > + if (!rtk_phy) {
> > + pr_err("%s rtk_phy is NULL!\n", __func__);
>
> What? How is this possible?
It should be not necessary. I will remove it.
> > + return -ENODEV;
> > + }
> > +
> > + dev_dbg(rtk_phy->dev, "Init RTK USB 3.0 PHY\n");
> > + for (i = 0; i < rtk_phy->phyN; i++)
> > + ret = do_rtk_usb_phy_init(rtk_phy, i);
> > +
> > + dev_info(rtk_phy->dev, "Initialized RTK USB 3.0 PHY (take %dms)\n",
> > + jiffies_to_msecs(jiffies - phy_init_time));
>
> Please drop all simple debug success messages. Linux has already
> infrastructure for this.
Ok, Thanks.
> > + return ret;
> > +}
> > +
> > +static int rtk_usb_phy_exit(struct phy *phy) {
> > + struct rtk_usb_phy *rtk_phy = phy_get_drvdata(phy);
> > +
> > + if (!rtk_phy) {
> > + pr_err("%s rtk_phy is NULL!\n", __func__);
> > + return -ENODEV;
> > + }
> > +
> > + dev_dbg(rtk_phy->dev, "Exit RTK USB 3.0 PHY\n");
>
> Please drop all simple debug success messages. Linux has already
> infrastructure for this.
Can I keep log for dev_dbg?
> > +static void rtk_usb_phy_toggle(struct usb_phy *usb3_phy, bool
> > +isConnect, int port) {
> > + int index = port;
> > + struct rtk_usb_phy *rtk_phy = NULL;
> > +
> > + if (usb3_phy != NULL && usb3_phy->dev != NULL)
> > + rtk_phy = dev_get_drvdata(usb3_phy->dev);
> > +
> > + if (rtk_phy == NULL) {
> > + pr_err("%s ERROR! NO this device\n", __func__);
>
> Your error messages are not helping. No need to shout, no need to handle
> some non-existing cases. If this is real case, you have broken driver. I actually
> suspect that.
>
> How can you interface with a driver where there is no device?
OK, I know this is not good programming practice, I will improve this question.
> > + return;
> > + }
> > +
> > + if (index > rtk_phy->phyN) {
> > + pr_err("%s %d ERROR! port=%d > phyN=%d\n",
> > + __func__, __LINE__, index, rtk_phy->phyN);
> > + return;
> > + }
> > +
> > + do_rtk_usb3_phy_toggle(rtk_phy, index, isConnect); }
> > +
> > +static int rtk_usb_phy_notify_port_status(struct usb_phy *x, int port,
> > + u16 portstatus, u16 portchange) {
> > + bool isConnect = false;
>
> This is not C++. Don't use camelcase. See Coding style document.
I will revised for this style.
> > +
> > + pr_debug("%s port=%d portstatus=0x%x portchange=0x%x\n",
> > + __func__, port, (int)portstatus, (int)portchange);
> > + if (portstatus & USB_PORT_STAT_CONNECTION)
> > + isConnect = true;
> > +
>
> ...
>
> > +
> > +static int rtk_usb3_set_parameter_show(struct seq_file *s, void
> > +*unused) {
> > + struct rtk_usb_phy *rtk_phy = s->private;
> > + const struct file *file = s->file;
> > + const char *file_name = file_dentry(file)->d_iname;
> > + struct dentry *p_dentry = file_dentry(file)->d_parent;
> > + const char *phy_dir_name = p_dentry->d_iname;
> > + int ret, index;
> > + struct phy_data *phy_data = NULL;
> > +
> > + for (index = 0; index < rtk_phy->phyN; index++) {
> > + size_t sz = 30;
> > + char name[30] = {0};
> > +
> > + snprintf(name, sz, "phy%d", index);
> > + if (strncmp(name, phy_dir_name, strlen(name)) == 0) {
> > + phy_data = &((struct phy_data
> *)rtk_phy->phy_data)[index];
> > + break;
> > + }
> > + }
> > + if (!phy_data) {
> > + dev_err(rtk_phy->dev,
> > + "%s: No phy_data for %s/%s\n",
> > + __func__, phy_dir_name,
> file_name);
>
> Mess wrapping/indentation. Actually everywhere in the file...
I will improve this.
> > +static int rtk_usb3_set_parameter_open(struct inode *inode, struct
> > +file *file) {
> > + return single_open(file, rtk_usb3_set_parameter_show,
> > +inode->i_private); }
> > +
> > +static ssize_t rtk_usb3_set_parameter_write(struct file *file,
> > + const char __user *ubuf, size_t count, loff_t *ppos) {
> > + const char *file_name = file_dentry(file)->d_iname;
> > + struct dentry *p_dentry = file_dentry(file)->d_parent;
> > + const char *phy_dir_name = p_dentry->d_iname;
> > + struct seq_file *s = file->private_data;
> > + struct rtk_usb_phy *rtk_phy = s->private;
> > + struct reg_addr *regAddr = NULL;
> > + struct phy_data *phy_data = NULL;
> > + int ret = 0;
> > + char buffer[40] = {0};
> > + int index;
> > +
> > + if (copy_from_user(&buffer, ubuf,
> > + min_t(size_t, sizeof(buffer) - 1, count)))
> > + return -EFAULT;
> > +
> > + for (index = 0; index < rtk_phy->phyN; index++) {
> > + size_t sz = 30;
> > + char name[30] = {0};
> > +
> > + snprintf(name, sz, "phy%d", index);
> > + if (strncmp(name, phy_dir_name, strlen(name)) == 0) {
> > + regAddr = &((struct reg_addr
> *)rtk_phy->reg_addr)[index];
> > + phy_data = &((struct phy_data
> *)rtk_phy->phy_data)[index];
> > + break;
>
>
> Where is the ABI documentation for user interface?
Do debugfs nodes need ABI documentation?
Is there a reference?
>
> > +
> > +static inline void create_debug_files(struct rtk_usb_phy *rtk_phy) {
> > + struct dentry *phy_debug_root = NULL;
> > + struct dentry *set_parameter_dir = NULL;
> > +
> > + phy_debug_root = create_phy_debug_root();
> > +
> > + if (!phy_debug_root) {
> > + dev_err(rtk_phy->dev, "%s Error phy_debug_root is NULL",
> > + __func__);
> > + return;
> > + }
> > + rtk_phy->debug_dir = debugfs_create_dir(dev_name(rtk_phy->dev),
> > + phy_debug_root);
> > + if (!rtk_phy->debug_dir) {
> > + dev_err(rtk_phy->dev, "%s Error debug_dir is NULL",
> > + __func__);
>
> Are you sure you run checkpatch on this? Error messages on debugfs are
> almost always incorrect.
Yes, I have run checkpatch for patches.
Why the message is incorrect?
> > +static int get_phy_parameter(struct rtk_usb_phy *rtk_phy,
> > + struct device_node *sub_node) {
> > + struct device *dev = rtk_phy->dev;
> > + struct reg_addr *addr;
> > + struct phy_data *phy_data;
> > + int ret = 0;
> > + int index;
> > +
> > + if (of_property_read_u32(sub_node, "reg", &index)) {
> > + dev_err(dev, "sub_node without reg\n");
> > + return -EINVAL;
> > + }
> > +
> > + dev_dbg(dev, "sub_node index=%d\n", index);
>
> Please drop all simple debug success messages. Linux has already
> infrastructure for this.
Can I keep log for dev_dbg?
> ...
>
> > +
> > +static int rtk_usb3phy_probe(struct platform_device *pdev) {
> > + struct rtk_usb_phy *rtk_phy;
> > + struct device *dev = &pdev->dev;
> > + struct device_node *node;
> > + struct device_node *sub_node;
> > + struct phy *generic_phy;
> > + struct phy_provider *phy_provider;
> > + int ret, phyN;
> > +
> > + 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-usb3phy";
> > + rtk_phy->phy.notify_port_status =
> > + rtk_usb_phy_notify_port_status;
> > +
> > + if (!dev->of_node) {
> > + dev_err(dev, "%s %d No device node\n", __func__,
> > + __LINE__);
>
> How is it even possible? If you do not have device node here, how did it probe?
You are right. The of_node is always exist.
I will remove it.
>
> > + ret = -ENODEV;
> > + goto err;
> > + }
> > +
> > + node = dev->of_node;
> > +
> > + phyN = of_get_child_count(node);
> > + rtk_phy->phyN = phyN;
> > + dev_dbg(dev, "%s phyN=%d\n", __func__, rtk_phy->phyN);
>
> Please drop all simple debug success messages. Linux has already
> infrastructure for this.
Okay. I will remove debug message.
Thanks,
Stanley
Powered by blists - more mailing lists