linux-kernel - Re: [PATCH v8 2/7] usb: mux: add generic code for dual role port mux

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

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20160525110652.GA27570@kuha.fi.intel.com>
Date:	Wed, 25 May 2016 14:06:52 +0300
From:	Heikki Krogerus <heikki.krogerus@...ux.intel.com>
To:	Lu Baolu <baolu.lu@...ux.intel.com>
Cc:	felipe.balbi@...ux.intel.com,
	Mathias Nyman <mathias.nyman@...el.com>,
	Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
	Lee Jones <lee.jones@...aro.org>,
	Liam Girdwood <lgirdwood@...il.com>,
	Mark Brown <broonie@...nel.org>, linux-usb@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH v8 2/7] usb: mux: add generic code for dual role port mux

Hi Baolu,

Sorry to comment this so late, but we got hardware that needs to
configure the mux in OS, and I noticed some problem. We are missing
means to bind a port to the correct mux on multiport systems. That we
need to solve later in any case, but there is an other issue related
to the fact that the notifiers now have to be extcon devices. It's
related, as extcon offers no means to solve the multiport issue, but
in any case..

> +struct portmux_dev *portmux_register(struct portmux_desc *desc)
> +{
> +	static atomic_t portmux_no = ATOMIC_INIT(-1);
> +	struct portmux_dev *pdev;
> +	struct extcon_dev *edev = NULL;
> +	struct device *dev;
> +	int ret;
> +
> +	/* parameter sanity check */
> +	if (!desc || !desc->name || !desc->ops || !desc->dev ||
> +	    !desc->ops->cable_set_cb || !desc->ops->cable_unset_cb)
> +		return ERR_PTR(-EINVAL);
> +
> +	dev = desc->dev;
> +
> +	if (desc->extcon_name) {
> +		edev = extcon_get_extcon_dev(desc->extcon_name);
> +		if (IS_ERR_OR_NULL(edev))
> +			return ERR_PTR(-EPROBE_DEFER);
> +	}
> +
> +	pdev = kzalloc(sizeof(*pdev), GFP_KERNEL);
> +	if (!pdev)
> +		return ERR_PTR(-ENOMEM);
> +
> +	pdev->desc = desc;
> +	pdev->edev = edev;
> +	pdev->nb.notifier_call = usb_mux_notifier;
> +	mutex_init(&pdev->mux_mutex);
> +
> +	pdev->dev.parent = dev;
> +	dev_set_name(&pdev->dev, "portmux.%lu",
> +		     (unsigned long)atomic_inc_return(&portmux_no));
> +	pdev->dev.groups = portmux_group;
> +	ret = device_register(&pdev->dev);
> +	if (ret)
> +		goto cleanup_mem;
> +
> +	dev_set_drvdata(&pdev->dev, pdev);
> +
> +	if (edev) {
> +		ret = extcon_register_notifier(edev, EXTCON_USB_HOST,
> +					       &pdev->nb);
> +		if (ret < 0) {
> +			dev_err(dev, "failed to register extcon notifier\n");
> +			goto cleanup_dev;
> +		}
> +	}

So I don't actually think this is correct approach. We are forcing the
notifying drivers, on top of depending on this framework, depend on
extcon too, and that simply is too much. I don't think a USB PHY or
charger detection driver should be forced to generate an extcon device
just to satisfy the mux in general.

Instead IMO, this framework should provide an API also for the
notifiers. The drivers that do the notification should not need to
depend on extcon at all. Instead they should be able to just request
an optional handle to a portmux, and use it with the function that you
already provide (usb_mux_change_state(), which of course needs to be
exported). That would make it much easier for us to make problems like
figuring out the correct mux for the correct port a problem for the
framework and not the drivers.

extcon does not really add any value here, but it does complicate
things a lot. We are even exposing new sysfs attributes to control the
mux, complete separate from extcon.

Thanks,

-- 
heikki