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: Fri, 7 Feb 2020 12:28:00 -0800 From: Prashant Malani <pmalani@...omium.org> To: Enric Balletbo i Serra <enric.balletbo@...labora.com> Cc: Linux Kernel Mailing List <linux-kernel@...r.kernel.org>, heikki.krogerus@...el.com, Benson Leung <bleung@...omium.org>, Guenter Roeck <groeck@...omium.org> Subject: Re: [PATCH 1/3] platform/chrome: Add Type C connector class driver On Fri, Feb 7, 2020 at 9:00 AM Enric Balletbo i Serra <enric.balletbo@...labora.com> wrote: > > Hi Prashant, > > On 7/2/20 0:30, Prashant Malani wrote: > > Hi Enric, > > > > Thanks for taking a look at the patch. Please see my responses inline > > (I will defer sending the next version till the one pending question I > > had is resolved): > > > > On Thu, Feb 6, 2020 at 8:19 AM Enric Balletbo i Serra > > <enric.balletbo@...labora.corp-partner.google.com> wrote: > >> > >> Hi Prashant, > >> > >> On 5/2/20 21:59, Prashant Malani wrote: > >>> Add a driver to implement the Type C connector class for Chrome OS > >>> devices with ECs (Embedded Controllers). > >>> > >>> The driver relies on firmware device specifications for various port > >>> attributes. On ACPI platforms, this is specified using the logical > >>> device with HID GOOG0014. On DT platforms, this is specified using the > >>> DT node with compatible string "google,cros-ec-typec". > >>> > >> > >> If that's the case you should document this in a binding file. > > > > Done. I will add this in the next version of the series. > > > >> driver a replacement of the cros-ec-extcon driver or is a different thing? > > > > Currently it is distinct. We're hoping to plug in to type C connector > > class work which Heikki is working on (relating to Type C Mux agent). > > Hopefully, we will gradually transition the extcon functionality to > > the Type C port driver. > > > >> > >> There is a device where I can test this? > > > > I think you can try it on kevin if you add the DT node for it (haven't > > tried it myself on kevin). An example will be present in the DT > > Documentation I provide in the next version. > > > >> > >>> This patch reads the device FW node and uses the port attributes to > >>> register the typec ports with the Type C connector class framework, but > >>> doesn't do much else. > >>> > >>> Subsequent patches will add more functionality to the driver, including > >>> obtaining current port information (polarity, vconn role, current power > >>> role etc.) after querying the EC. > >>> > >>> Signed-off-by: Prashant Malani <pmalani@...omium.org> > >>> --- > >>> drivers/platform/chrome/Kconfig | 11 ++ > >>> drivers/platform/chrome/Makefile | 1 + > >>> drivers/platform/chrome/cros_ec_typec.c | 228 ++++++++++++++++++++++++ > >>> 3 files changed, 240 insertions(+) > >>> create mode 100644 drivers/platform/chrome/cros_ec_typec.c > >>> > >>> diff --git a/drivers/platform/chrome/Kconfig b/drivers/platform/chrome/Kconfig > >>> index 5f57282a28da00..1370dfd1ca1565 100644 > >>> --- a/drivers/platform/chrome/Kconfig > >>> +++ b/drivers/platform/chrome/Kconfig > >>> @@ -214,6 +214,17 @@ config CROS_EC_SYSFS > >>> To compile this driver as a module, choose M here: the > >>> module will be called cros_ec_sysfs. > >>> > >>> +config CROS_EC_TYPEC > >>> + tristate "ChromeOS EC Type-C Connector Control" > >>> + depends on MFD_CROS_EC_DEV && TYPEC > >>> + default n > >> > >> Default value is already n, so you don't need to put it here. But I'd say that > >> we might be interested on have default MFD_CROS_EC_DEV like the other drivers. > > > > Done. > > > >> > >>> + help > >>> + If you say Y here, you get support for accessing Type C connector > >>> + information from the Chrome OS EC. > >>> + > >>> + To compile this driver as a module, choose M here: the module will be > >>> + called cros_ec_typec. > >>> + > >>> config CROS_USBPD_LOGGER > >>> tristate "Logging driver for USB PD charger" > >>> depends on CHARGER_CROS_USBPD > >>> diff --git a/drivers/platform/chrome/Makefile b/drivers/platform/chrome/Makefile > >>> index aacd5920d8a180..caf2a9cdb5e6d1 100644 > >>> --- a/drivers/platform/chrome/Makefile > >>> +++ b/drivers/platform/chrome/Makefile > >>> @@ -12,6 +12,7 @@ obj-$(CONFIG_CROS_EC_ISHTP) += cros_ec_ishtp.o > >>> obj-$(CONFIG_CROS_EC_RPMSG) += cros_ec_rpmsg.o > >>> obj-$(CONFIG_CROS_EC_SPI) += cros_ec_spi.o > >>> cros_ec_lpcs-objs := cros_ec_lpc.o cros_ec_lpc_mec.o > >>> +obj-$(CONFIG_CROS_EC_TYPEC) += cros_ec_typec.o > >>> obj-$(CONFIG_CROS_EC_LPC) += cros_ec_lpcs.o > >>> obj-$(CONFIG_CROS_EC_PROTO) += cros_ec_proto.o cros_ec_trace.o > >>> obj-$(CONFIG_CROS_KBD_LED_BACKLIGHT) += cros_kbd_led_backlight.o > >>> diff --git a/drivers/platform/chrome/cros_ec_typec.c b/drivers/platform/chrome/cros_ec_typec.c > >>> new file mode 100644 > >>> index 00000000000000..fe5659171c2a85 > >>> --- /dev/null > >>> +++ b/drivers/platform/chrome/cros_ec_typec.c > >>> @@ -0,0 +1,228 @@ > >>> +// SPDX-License-Identifier: GPL-2.0-only > >>> +/* > >>> + * Copyright 2020 Google LLC > >>> + * > >>> + * This driver provides the ability to view and manage Type C ports through the > >>> + * Chrome OS EC. > >>> + */ > >>> + > >>> +#include <linux/acpi.h> > >>> +#include <linux/module.h> > >>> +#include <linux/of.h> > >>> +#include <linux/platform_data/cros_ec_commands.h> > >>> +#include <linux/platform_data/cros_ec_proto.h> > >>> +#include <linux/platform_device.h> > >>> +#include <linux/usb/typec.h> > >>> + > >>> +#define DRV_NAME "cros-ec-typec" > >>> + > >>> +/* Platform-specific data for the Chrome OS EC Type C controller. */ > >>> +struct cros_typec_data { > >>> + struct device *dev; > >>> + struct cros_ec_device *ec; > >>> + int num_ports; > >>> + /* Array of ports, indexed by port number. */ > >>> + struct typec_port *ports[EC_USB_PD_MAX_PORTS]; > >>> +}; > >>> + > >>> +int cros_typec_parse_port_props(struct typec_capability *cap, > >> > >> static int > > > > Sorry, done. > > > >> > >>> + struct fwnode_handle *fwnode, > >>> + struct device *dev) > >>> +{ > >>> + const char *buf; > >>> + int ret; > >>> + > >>> + memset(cap, 0, sizeof(*cap)); > >>> + ret = fwnode_property_read_string(fwnode, "power-role", &buf); > >>> + if (ret) { > >>> + dev_err(dev, "power-role not found: %d\n", ret); > >>> + return ret; > >>> + } > >>> + > >>> + ret = typec_find_port_power_role(buf); > >>> + if (ret < 0) > >>> + return ret; > >>> + cap->type = ret; > >>> + > >>> + ret = fwnode_property_read_string(fwnode, "data-role", &buf); > >>> + if (ret) { > >>> + dev_err(dev, "data-role not found: %d\n", ret); > >>> + return ret; > >>> + } > >>> + > >>> + ret = typec_find_port_data_role(buf); > >>> + if (ret < 0) > >>> + return ret; > >>> + cap->data = ret; > >>> + > >>> + ret = fwnode_property_read_string(fwnode, "try-power-role", &buf); > >>> + if (ret) { > >>> + dev_err(dev, "try-power-role not found: %d\n", ret); > >>> + return ret; > >>> + } > >>> + > >>> + ret = typec_find_power_role(buf); > >>> + if (ret < 0) > >>> + return ret; > >>> + cap->prefer_role = ret; > >>> + > >>> + cap->fwnode = fwnode; > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static int cros_typec_init_ports(struct cros_typec_data *typec) > >>> +{ > >>> + struct device *dev = typec->dev; > >>> + struct typec_capability cap; > >>> + struct fwnode_handle *fwnode; > >>> + int ret; > >>> + int i; > >>> + int nports; > >>> + u32 port_num; > >>> + > >>> + nports = device_get_child_node_count(dev); > >>> + if (nports == 0) { > >>> + dev_err(dev, "No port entries found.\n"); > >>> + return -ENODEV; > >>> + } > >>> + > >>> + device_for_each_child_node(dev, fwnode) { > >>> + if (fwnode_property_read_u32(fwnode, "port-number", > >>> + &port_num)) { > >>> + dev_err(dev, "No port-number for port, skipping.\n"); > >>> + ret = -EINVAL; > >>> + goto unregister_ports; > >>> + } > >>> + > >>> + if (port_num >= typec->num_ports) { > >>> + dev_err(dev, "Invalid port number.\n"); > >>> + ret = -EINVAL; > >>> + goto unregister_ports; > >>> + } > >>> + > >>> + dev_dbg(dev, "Registering port %d\n", port_num); > >>> + ret = cros_typec_parse_port_props(&cap, fwnode, dev); > >>> + if (ret < 0) > >>> + goto unregister_ports; > >>> + typec->ports[port_num] = typec_register_port(dev, &cap); > >>> + if (IS_ERR(typec->ports[port_num])) { > >>> + dev_err(dev, "Failed to register port %d\n", port_num); > >>> + ret = PTR_ERR(typec->ports[port_num]); > >>> + goto unregister_ports; > >>> + } > >>> + } > >>> + > >>> + return 0; > >>> + > >>> +unregister_ports: > >>> + for (i = 0; i < typec->num_ports; i++) > >>> + typec_unregister_port(typec->ports[i]); > >>> + return ret; > >>> +} > >>> + > >>> +static int cros_typec_ec_command(struct cros_typec_data *typec, > >>> + unsigned int version, > >>> + unsigned int command, > >>> + void *outdata, > >>> + unsigned int outsize, > >>> + void *indata, > >>> + unsigned int insize) > >>> +{ > >>> + struct cros_ec_command *msg; > >>> + int ret; > >>> + > >>> + msg = kzalloc(sizeof(*msg) + max(outsize, insize), GFP_KERNEL); > >>> + if (!msg) > >>> + return -ENOMEM; > >>> + > >>> + msg->version = version; > >>> + msg->command = command; > >>> + msg->outsize = outsize; > >>> + msg->insize = insize; > >>> + > >>> + if (outsize) > >>> + memcpy(msg->data, outdata, outsize); > >>> + > >>> + ret = cros_ec_cmd_xfer_status(typec->ec, msg); > >>> + if (ret >= 0 && insize) > >>> + memcpy(indata, msg->data, insize); > >>> + > >>> + kfree(msg); > >>> + return ret; > >>> +} > >>> + > >>> + > >>> +static int cros_typec_get_num_ports(struct cros_typec_data *typec) > >>> +{ > >> > >> This functions is only called once at probe, you can just put the code there. I > >> had some readibility problems trying to follow de code. > > > > Done. > > > >> > >>> + struct ec_response_usb_pd_ports resp; > >>> + int ret; > >>> + > >>> + ret = cros_typec_ec_command(typec, 0, EC_CMD_USB_PD_PORTS, NULL, 0, > >>> + &resp, sizeof(resp)); > >>> + if (ret < 0) > >>> + return ret; > >>> + > >>> + typec->num_ports = resp.num_ports; > >>> + if (typec->num_ports > EC_USB_PD_MAX_PORTS) { > >>> + dev_warn(typec->dev, > >>> + "Too many ports reported: %d, limiting to max: %d\n", > >>> + typec->num_ports, EC_USB_PD_MAX_PORTS); > >> > >> You say that you are limiting the number of ports to max but typec->num_ports is > >> still resp.num_ports, is that correct? > >> > > > > Sorry, thanks for catching this, it should be set to > > EC_USB_PD_MAX_PORTS. I will fix this in the next version. > > > >>> + } > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +#ifdef CONFIG_ACPI > >>> +static const struct acpi_device_id cros_typec_acpi_id[] = { > >>> + { "GOOG0014", 0 }, > >>> + { /* sentinel */ } > >> > >> No need to add /* sentinel */ here, is obvious. > > > > Done. > > > >> > >>> +}; > >>> +MODULE_DEVICE_TABLE(acpi, cros_typec_acpi_id); > >>> +#endif > >>> + > >>> +#ifdef CONFIG_OF > >>> +static const struct of_device_id cros_typec_of_match[] = { > >>> + { .compatible = "google,cros-ec-typec", }, > >>> + { /* sentinel */ }, > >> > >> No need to add /* sentinel */ here, is obvious. And no need for the colon at the > >> end. > > > > Done. > >> > >>> +}; > >>> +MODULE_DEVICE_TABLE(of, cros_typec_of_match); > >>> +#endif > >>> + > >>> +static int cros_typec_probe(struct platform_device *pdev) > >>> +{ > >>> + struct device *dev = &pdev->dev; > >>> + struct cros_typec_data *typec; > >>> + int ret; > >>> + > >>> + typec = devm_kzalloc(dev, sizeof(*typec), GFP_KERNEL); > >>> + if (!typec) > >>> + return -ENOMEM; > >>> + typec->dev = dev; > >>> + typec->ec = dev_get_drvdata(pdev->dev.parent); > >>> + platform_set_drvdata(pdev, typec); > >>> + > >>> + ret = cros_typec_get_num_ports(typec); > >>> + if (ret < 0) > >>> + return ret; > >>> + > >>> + ret = cros_typec_init_ports(typec); > >> > >> both, cros_typec_get_num_ports and cros_typec_init_ports are only called once, > >> as I said I had some problems of readibility because typec->num_ports is set in > >> one function and unregister in another function when fails. > > > > Can we keep cros_typec_init_ports() as a separate function? I was > > hoping to avoid cluttering the _probe() with FW node parsing logic. > > cros_typec_init_ports() registers the ports (and unregisters on > > failure), so seems fairly self-contained. > > Of course, happy to place everything in probe if you still prefer that. > > > > I don't mind clutter probe if is more readable but try it, and lets see how > looks like. You are also registering and unregistering the typec ports there, > not only parsing. Usually what we have is parsing the firmware or dt and then > register and unregister the typec ports, all mixed. I'm more in favour of have > it separated, IMHO. Sounds good, I'll push another version with cros_typec_init_ports() distinct and we can check how it looks. If it's still not readable, I'll push version with the code directly in probe() > > >> > >> I'd just remove those two functions and put the code directly here. > >> > >>> + if (!ret) > >>> + return ret; > >>> + > >>> + return 0; > >>> +} > >>> + > >>> +static struct platform_driver cros_typec_driver = { > >>> + .driver = { > >>> + .name = DRV_NAME, > >> > >> no tab, just space after .-name > > > > Done. > >> > >>> + .acpi_match_table = ACPI_PTR(cros_typec_acpi_id), > >>> + .of_match_table = of_match_ptr(cros_typec_of_match), > >>> + }, > >>> + .probe = cros_typec_probe, > >> > >> no tab, just space after .probe > > > > Done. > > > >> > >>> +}; > >>> + > >>> +module_platform_driver(cros_typec_driver); > >>> + > >>> +MODULE_LICENSE("GPL"); > >>> +MODULE_DESCRIPTION("Chrome OS EC Type C control"); > >>> > >> > >> You probably want to add the MODULE_AUTHOR here > > > > Done. > > > > Best regards, > >
Powered by blists - more mailing lists