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 X-Spam-Level: X-Spam-Status: No, score=-6.3 required=3.0 tests=BAYES_00,DKIMWL_WL_HIGH, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HEADER_FROM_DIFFERENT_DOMAINS, MAILING_LIST_MULTI,SPF_HELO_NONE,SPF_PASS autolearn=no autolearn_force=no version=3.4.0 Received: from mail.kernel.org (mail.kernel.org [198.145.29.99]) by smtp.lore.kernel.org (Postfix) with ESMTP id 29A11C433E0 for ; Thu, 11 Feb 2021 18:18:20 +0000 (UTC) Received: from vger.kernel.org (vger.kernel.org [23.128.96.18]) by mail.kernel.org (Postfix) with ESMTP id DFBB164E65 for ; Thu, 11 Feb 2021 18:18:19 +0000 (UTC) Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S232664AbhBKSHB (ORCPT ); Thu, 11 Feb 2021 13:07:01 -0500 Received: from lindbergh.monkeyblade.net ([23.128.96.19]:58064 "EHLO lindbergh.monkeyblade.net" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S232530AbhBKSEx (ORCPT ); Thu, 11 Feb 2021 13:04:53 -0500 Received: from mail-pf1-x42a.google.com (mail-pf1-x42a.google.com [IPv6:2607:f8b0:4864:20::42a]) by lindbergh.monkeyblade.net (Postfix) with ESMTPS id E3188C061786 for ; Thu, 11 Feb 2021 10:04:11 -0800 (PST) Received: by mail-pf1-x42a.google.com with SMTP id k13so4141462pfh.13 for ; Thu, 11 Feb 2021 10:04:11 -0800 (PST) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to; bh=ABmGACKj8AbXK76X6Oa7fcPA01DfnOcIma2NBDr+Ii8=; b=gdDg+trAg1mnOAYW198dpCWFTvXYocL6QHdOt952qYu1iFidwX9K64iSDFdFlMpaas /UPo99v23QnfYtMzFZCBYxt7DtkTQyRdQq0zB/TE8/VxDRKXUYnxvf4Ncf1zAer5315c 5JSozgsGTijUu53setjDkpshgqAIqJpq6Jvow= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20161025; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to; bh=ABmGACKj8AbXK76X6Oa7fcPA01DfnOcIma2NBDr+Ii8=; b=R9GTjHFnsm8M0Q1FwIzJAP9ohxoQga7kITmvy7QCGKLoNZ3qM/8a523g4kzRI/K4Rn 59is9W2YYKlMEzUxsptISFcXmDvbcfYrDTFY8VdEmFgnSpnPSTw4GgPtmnEzRW/eHZ1w cNg0RMvJerKYYyjCpTxS94pCLFMi9XGp2gq1jQQeJ9QKXvWGRmu7XSe7qvWO8lsYA9hF 85CemhHKEM6xjf3QTSzaD3IyM3BSjQ7hTT2oiLq4Hlc70/hZ2zd3maHaNSS2G665eev6 4c94JWnFyx5Kl2uW66qo81KPe6YW5JdnucZVFAg0JhxA7Rj0y4vBVYH+OPCAewcM3hVT b33w== X-Gm-Message-State: AOAM532isr+bQPeXvglTYgU5GPAMWBGh/MMqZym7DWaEQMtrtwL1jsgm HTz8K1U6d5JDtFx1LW5te7H0ZQ== X-Google-Smtp-Source: ABdhPJxgeZDClYTwM7w3KSD3XpIZZ3WMRSb8LUUqNHFWQ2sSL1PVnza2+m10uol/5oUW2d5jDTZqrA== X-Received: by 2002:a05:6a00:22d6:b029:1cb:35ac:d8e0 with SMTP id f22-20020a056a0022d6b02901cb35acd8e0mr8938502pfj.17.1613066651431; Thu, 11 Feb 2021 10:04:11 -0800 (PST) Received: from localhost ([2620:15c:202:1:fc92:99c:fc2f:8603]) by smtp.gmail.com with UTF8SMTPSA id h11sm3967858pjc.27.2021.02.11.10.04.09 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Thu, 11 Feb 2021 10:04:10 -0800 (PST) Date: Thu, 11 Feb 2021 10:04:08 -0800 From: Matthias Kaehlcke To: Greg Kroah-Hartman Cc: Rob Herring , Frank Rowand , devicetree@vger.kernel.org, Peter Chen , Stephen Boyd , Alan Stern , Ravi Chandra Sadineni , Bastien Nocera , linux-kernel@vger.kernel.org, Douglas Anderson , linux-usb@vger.kernel.org, Krzysztof Kozlowski , Al Cooper , "Alexander A. Klimov" , Masahiro Yamada Subject: Re: [PATCH v5 2/4] USB: misc: Add onboard_usb_hub driver Message-ID: References: <20210210171040.684659-1-mka@chromium.org> <20210210091015.v5.2.I7c9a1f1d6ced41dd8310e8a03da666a32364e790@changeid> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Disposition: inline In-Reply-To: Precedence: bulk List-ID: X-Mailing-List: linux-usb@vger.kernel.org Hi Greg, On Thu, Feb 11, 2021 at 08:03:01AM +0100, Greg Kroah-Hartman wrote: > On Wed, Feb 10, 2021 at 09:10:37AM -0800, Matthias Kaehlcke wrote: > > +static int onboard_hub_add_usbdev(struct onboard_hub *hub, struct usb_device *udev) > > +{ > > + struct udev_node *node; > > + char link_name[64]; > > + int ret = 0; > > + > > + mutex_lock(&hub->lock); > > + > > + if (hub->going_away) { > > + ret = -EINVAL; > > + goto unlock; > > + } > > + > > + node = devm_kzalloc(hub->dev, sizeof(*node), GFP_KERNEL); > > + if (!node) { > > + ret = -ENOMEM; > > + goto unlock; > > + } > > + > > + node->udev = udev; > > + > > + list_add(&node->list, &hub->udev_list); > > + > > + snprintf(link_name, sizeof(link_name), "usb_dev.%s", dev_name(&udev->dev)); > > + WARN_ON(sysfs_create_link(&hub->dev->kobj, &udev->dev.kobj, link_name)); > > Never use WARN_ON() unless you want the machine to reboot if it triggers > (panic on warn). Ah, thanks, I wasn't aware of that. Will change to some type of log if the sysfs attributes stick around. > But the larger question is what is this sysfs link for? It's not > documented anywhere, and so, shouldn't be allowed. Who is going to use > it and why is it needed? Alan asked to add them: https://lore.kernel.org/patchwork/patch/1313000/#1514338 I'm fine with either way, let's just agree on something :) > > + > > +unlock: > > + mutex_unlock(&hub->lock); > > + > > + return ret; > > +} > > + > > +static void onboard_hub_remove_usbdev(struct onboard_hub *hub, struct usb_device *udev) > > +{ > > + struct udev_node *node; > > + char link_name[64]; > > + > > + snprintf(link_name, sizeof(link_name), "usb_dev.%s", dev_name(&udev->dev)); > > + sysfs_remove_link(&hub->dev->kobj, link_name); > > + > > + mutex_lock(&hub->lock); > > + > > + list_for_each_entry(node, &hub->udev_list, list) { > > + if (node->udev == udev) { > > + list_del(&node->list); > > + break; > > + } > > + } > > + > > + mutex_unlock(&hub->lock); > > +} > > + > > +static ssize_t always_powered_in_suspend_show(struct device *dev, struct device_attribute *attr, > > + char *buf) > > +{ > > + struct onboard_hub *hub = dev_get_drvdata(dev); > > + > > + return sprintf(buf, "%d\n", hub->always_powered_in_suspend); > > sysfs_emit()? ok > And you forgot the Documentation/ABI/ entries for this driver, so it > really can not be reviewed... I'll add it in the next version. > > +} > > + > > +static ssize_t always_powered_in_suspend_store(struct device *dev, struct device_attribute *attr, > > + const char *buf, size_t count) > > +{ > > + struct onboard_hub *hub = dev_get_drvdata(dev); > > + bool val; > > + int ret; > > + > > + ret = kstrtobool(buf, &val); > > + if (ret < 0) > > + return ret; > > + > > + hub->always_powered_in_suspend = val; > > + > > + return count; > > +} > > +static DEVICE_ATTR_RW(always_powered_in_suspend); > > + > > +static struct attribute *onboard_hub_sysfs_entries[] = { > > + &dev_attr_always_powered_in_suspend.attr, > > + NULL, > > +}; > > + > > +static const struct attribute_group onboard_hub_sysfs_group = { > > + .attrs = onboard_hub_sysfs_entries, > > +}; > > + > > +static int onboard_hub_probe(struct platform_device *pdev) > > +{ > > + struct device *dev = &pdev->dev; > > + struct onboard_hub *hub; > > + int err; > > + > > + hub = devm_kzalloc(dev, sizeof(*hub), GFP_KERNEL); > > + if (!hub) > > + return -ENOMEM; > > + > > + hub->vdd = devm_regulator_get(dev, "vdd"); > > + if (IS_ERR(hub->vdd)) > > + return PTR_ERR(hub->vdd); > > + > > + hub->dev = dev; > > + mutex_init(&hub->lock); > > + INIT_LIST_HEAD(&hub->udev_list); > > + > > + dev_set_drvdata(dev, hub); > > + > > + err = devm_device_add_group(dev, &onboard_hub_sysfs_group); > > You just raced userspace and lost :( > > Please use the correct api to add sysfs attributes to the device > automatically by the driver core. ok > But the larger question is why do you need them at all? What do they > do that we can't already do with existing apis that you feel a one-off > api for this driver is required? The 'always_powered_in_suspend' attribute allows the admin to configure whether the hub should always been kept on in suspend. I'm know about existing APIs that would be suitable for that, but that might be just my ignorance. If you have any suggestions please let me know. > > + if (err) { > > + dev_err(dev, "failed to create sysfs entries: %d\n", err); > > + return err; > > + } > > + > > + err = onboard_hub_power_on(hub); > > + if (err) > > + return err; > > + > > + /* > > + * The USB driver might have been detached from the USB devices by > > + * onboard_hub_remove() make sure to re-attach it if needed. > > + */ > > + (void)driver_attach(&onboard_hub_usbdev_driver.drvwrap.driver); > > (void)???? > > Please no, do it right. Ok, driver_attach() does not return an error when the driver is already attached, so it should be fine to change this to an error check. > But, why is a driver calling this function anyway? That feels really > really wrong... I found during testing that this is needed to make sure the driver is attached again after it was released in onboard_hub_remove(). Alan requested to release the driver to avoid dangling references: https://lore.kernel.org/patchwork/patch/1310889/#1506598 Thanks Matthias