From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by smtp.lore.kernel.org (Postfix) with ESMTP id 87BD2C7EE25 for ; Fri, 9 Jun 2023 09:31:05 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S241635AbjFIJbD (ORCPT ); Fri, 9 Jun 2023 05:31:03 -0400 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:52186 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S241640AbjFIJap (ORCPT ); Fri, 9 Jun 2023 05:30:45 -0400 Received: from dfw.source.kernel.org (dfw.source.kernel.org [IPv6:2604:1380:4641:c500::1]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id 9B53A4C3F; Fri, 9 Jun 2023 02:25:28 -0700 (PDT) Received: from smtp.kernel.org (relay.kernel.org [52.25.139.140]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by dfw.source.kernel.org (Postfix) with ESMTPS id 9E4B4655C1; Fri, 9 Jun 2023 09:25:18 +0000 (UTC) Received: by smtp.kernel.org (Postfix) with ESMTPSA id B41F3C433EF; Fri, 9 Jun 2023 09:25:17 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=linuxfoundation.org; s=korg; t=1686302718; bh=nSdYW+WFsl6MvtTLdvkbfZTNXmGHGaeOLx9kkCj8tHA=; h=Date:From:To:Cc:Subject:References:In-Reply-To:From; b=YsoA74TU0rXqPGa5w/+qVtUH9xY618Yf92W+zjj3lVPgpIAA6sqAVqPnpfqRLXqXQ An5yFYQrZ6FfXa/nfzIhhFfL5kSwCOYiWML5GeGkEl6RlQzddMs6uZvtUINe+SiOX/ g3GThdNdvfEiz/EunIjtRYTqZ+pupBzHABHaQP0E= Date: Fri, 9 Jun 2023 11:25:15 +0200 From: Greg Kroah-Hartman To: zhuyinbo Cc: Minas Harutyunyan , Alan Stern , linux-usb@vger.kernel.org, linux-kernel@vger.kernel.org, Jianmin Lv , wanghongliang@loongson.cn, Liu Peibao , loongson-kernel@lists.loongnix.cn Subject: Re: [PATCH v2] usb: dwc2: add pci_device_id driver_data parse support Message-ID: <2023060958-unvocal-flattery-256a@gregkh> References: <20230609025047.691-1-zhuyinbo@loongson.cn> <2023060915-uneasy-pedicure-35f4@gregkh> <1e2a07a4-f81f-3672-f29c-144d1a12ea21@loongson.cn> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1e2a07a4-f81f-3672-f29c-144d1a12ea21@loongson.cn> Precedence: bulk List-ID: X-Mailing-List: linux-usb@vger.kernel.org On Fri, Jun 09, 2023 at 05:07:03PM +0800, zhuyinbo wrote: > > > 在 2023/6/9 下午2:13, Greg Kroah-Hartman 写道: > > On Fri, Jun 09, 2023 at 10:50:47AM +0800, Yinbo Zhu wrote: > > > The dwc2 driver has everything we need to run in PCI mode except > > > for pci_device_id driver_data parse. With that to set Loongson > > > dwc2 element and added identified as PCI_VENDOR_ID_LOONGSON > > > and PCI_DEVICE_ID_LOONGSON_DWC2 in dwc2_pci_ids, the Loongson > > > dwc2 controller will work. > > > > > > Signed-off-by: Yinbo Zhu > > > --- > > > Change in v2: > > > 1. Move the dwc2 pci ID from pci_ids.h to params.c. > > > 2. Add some code logic to ensure that the current device is > > > a PCI device. > > > 3. Fix the compile issue when dwc2 pci driver as module. > > > > > > drivers/usb/dwc2/core.h | 1 + > > > drivers/usb/dwc2/params.c | 39 ++++++++++++++++++++++++++++++++++++++- > > > drivers/usb/dwc2/pci.c | 14 +------------- > > > 3 files changed, 40 insertions(+), 14 deletions(-) > > > > > > diff --git a/drivers/usb/dwc2/core.h b/drivers/usb/dwc2/core.h > > > index 0bb4c0c845bf..c92a1da46a01 100644 > > > --- a/drivers/usb/dwc2/core.h > > > +++ b/drivers/usb/dwc2/core.h > > > @@ -1330,6 +1330,7 @@ irqreturn_t dwc2_handle_common_intr(int irq, void *dev); > > > /* The device ID match table */ > > > extern const struct of_device_id dwc2_of_match_table[]; > > > extern const struct acpi_device_id dwc2_acpi_match[]; > > > +extern const struct pci_device_id dwc2_pci_ids[]; > > > int dwc2_lowlevel_hw_enable(struct dwc2_hsotg *hsotg); > > > int dwc2_lowlevel_hw_disable(struct dwc2_hsotg *hsotg); > > > diff --git a/drivers/usb/dwc2/params.c b/drivers/usb/dwc2/params.c > > > index 21d16533bd2f..6b68a8830781 100644 > > > --- a/drivers/usb/dwc2/params.c > > > +++ b/drivers/usb/dwc2/params.c > > > @@ -7,9 +7,14 @@ > > > #include > > > #include > > > #include > > > +#include > > > +#include > > > #include "core.h" > > > +#define PCI_PRODUCT_ID_HAPS_HSOTG 0xabc0 > > > +#define PCI_DEVICE_ID_LOONGSON_DWC2 0x7a04 > > > + > > > static void dwc2_set_bcm_params(struct dwc2_hsotg *hsotg) > > > { > > > struct dwc2_core_params *p = &hsotg->params; > > > @@ -55,6 +60,14 @@ static void dwc2_set_jz4775_params(struct dwc2_hsotg *hsotg) > > > !device_property_read_bool(hsotg->dev, "disable-over-current"); > > > } > > > +static void dwc2_set_loongson_params(struct dwc2_hsotg *hsotg) > > > +{ > > > + struct dwc2_core_params *p = &hsotg->params; > > > + > > > + p->phy_utmi_width = 8; > > > + p->power_down = DWC2_POWER_DOWN_PARAM_PARTIAL; > > > +} > > > + > > > static void dwc2_set_x1600_params(struct dwc2_hsotg *hsotg) > > > { > > > struct dwc2_core_params *p = &hsotg->params; > > > @@ -281,6 +294,23 @@ const struct acpi_device_id dwc2_acpi_match[] = { > > > }; > > > MODULE_DEVICE_TABLE(acpi, dwc2_acpi_match); > > > +const struct pci_device_id dwc2_pci_ids[] = { > > > + { > > > + PCI_DEVICE(PCI_VENDOR_ID_SYNOPSYS, PCI_PRODUCT_ID_HAPS_HSOTG), > > > + }, > > > + { > > > + PCI_DEVICE(PCI_VENDOR_ID_STMICRO, > > > + PCI_DEVICE_ID_STMICRO_USB_OTG), > > > + }, > > > + { > > > + PCI_DEVICE(PCI_VENDOR_ID_LOONGSON, PCI_DEVICE_ID_LOONGSON_DWC2), > > > + .driver_data = (unsigned long)dwc2_set_loongson_params, > > > + }, > > > + { /* end: all zeroes */ } > > > +}; > > > +MODULE_DEVICE_TABLE(pci, dwc2_pci_ids); > > > +EXPORT_SYMBOL_GPL(dwc2_pci_ids); > > > + > > > static void dwc2_set_param_otg_cap(struct dwc2_hsotg *hsotg) > > > { > > > switch (hsotg->hw_params.op_mode) { > > > @@ -927,13 +957,20 @@ int dwc2_init_params(struct dwc2_hsotg *hsotg) > > > if (match && match->data) { > > > set_params = match->data; > > > set_params(hsotg); > > > - } else { > > > + } else if (!match) { > > > const struct acpi_device_id *amatch; > > > + const struct pci_device_id *pmatch = NULL; > > > amatch = acpi_match_device(dwc2_acpi_match, hsotg->dev); > > > if (amatch && amatch->driver_data) { > > > set_params = (set_params_cb)amatch->driver_data; > > > set_params(hsotg); > > > + } else if (!amatch) > > > + pmatch = pci_match_id(dwc2_pci_ids, to_pci_dev(hsotg->dev->parent)); > > > > At this point in time, how can you guarantee that the parent device > > really is a PCI one? This function is being called from a platform > > device callback, and platform devices should NEVER be a child of a PCI > > device, as that's not how PCI or platform devices work. > > > > So how is this even possible? > > > > confused, > > > > > Hi greg k-h, > > My current considerations are based on that the dwc2 drivers support > three types of devices, they are Platform device / ACPI device / PCI > device, and The dwc2/platform.c can all cover the three type dwc2 > device, no matter how it is registered. > > So, when a dwc2 device wasn't platform device and acpi device and It > will be a PCI device, and the the dwc2/pci.c will register a dwc2 > platform device that dwc2 device is a device that in dwc2/platform.c's > dwc2_driver_probe. the "&dwc2->dev->parent" is the "hsotg->dev->parent" > and it was also the &pci->dev. That's wrong, a PCI device should NEVER register a platform device under it, as obviously it's not a platform device at all. So please work to fix that layering violation here, as that's not correct and will cause problems as you are showing here. > So, We can use "to_pci_dev(hsotg->dev->parent)" to gain a dwc2 pci > device. No, not if the parent is something else as you have no way of knowing this at this point in time. > > 1) DWC2 PCI device driver (drivers/usb/dwc2/pci.c) : > > static int dwc2_pci_probe(struct pci_dev *pci, > const struct pci_device_id *id) > { > ... > struct platform_device *dwc2; > ... > struct device *dev = &pci->dev; > ... > dwc2 = platform_device_alloc("dwc2", PLATFORM_DEVID_AUTO); > //"dwc2" was used to match dwc2 platform driver This needs to be fixed, it's not ok at all. Just use the real PCI device here please. > ... > dwc2->dev.parent = dev; > ... > > } > > 2) DWC2 PLATFROM driver (drivers/usb/dwc2/platform.c) : > // This driver cover pci type device. > > static const char dwc2_driver_name[] = "dwc2"; > static struct platform_driver dwc2_platform_driver = { > .driver = { > .name = dwc2_driver_name, > //"dwc2" was used to match pci type dwc2 device > ... > }, > .probe = dwc2_driver_probe, > .remove = dwc2_driver_remove, > .shutdown = dwc2_driver_shutdown, > }; > static int dwc2_driver_probe(struct platform_device *dev) > { > // The dev is the "dwc2" platform device in dwc2/pci.c driver. > struct dwc2_hsotg *hsotg; > ... > hsotg->dev = &dev->dev; Yes, and if this was a real platform device, the parent would NOT be a PCI device, and so your check will cause all sorts of problems, right? thanks, greg k-h