From patchwork Mon Sep 5 15:15:02 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Thierry Reding X-Patchwork-Id: 665898 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from vger.kernel.org (vger.kernel.org [209.132.180.67]) by ozlabs.org (Postfix) with ESMTP id 3sSYFC0Vq0z9s9c for ; Tue, 6 Sep 2016 01:15:09 +1000 (AEST) Authentication-Results: ozlabs.org; dkim=fail reason="signature verification failed" (2048-bit key; unprotected) header.d=gmail.com header.i=@gmail.com header.b=jGeSpj4x; dkim-atps=neutral Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754468AbcIEPPH (ORCPT ); Mon, 5 Sep 2016 11:15:07 -0400 Received: from mail-wm0-f66.google.com ([74.125.82.66]:35915 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752756AbcIEPPG (ORCPT ); Mon, 5 Sep 2016 11:15:06 -0400 Received: by mail-wm0-f66.google.com with SMTP id l65so6088645wmf.3; Mon, 05 Sep 2016 08:15:05 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20120113; h=date:from:to:cc:subject:message-id:references:mime-version :content-disposition:in-reply-to:user-agent; bh=WjuQ6uvglX+BhkgNWK/xcfqgZsP6l/4QSb/FmG8wG/A=; b=jGeSpj4xcKh0DQ0In/qtcDOvxjQ8/gJ2jUcNYJxMIdfnS3odlOUNPJhDuLdNxuPDAn XxBu6bYRF8gl508RWK7mhfFb5qyLcvfR76HXZ3XjCe9Bb+0y8XLnKVW9UcMSRnuEomLF b4GPvVrLoFVfIzx9UEqwkQQAyYsictDd78kWrziRTQ4M9u7mNC/u0RLN4hONTW4o21aS 1EkaIyhWvP/LT7K0ifkWzDIAfd171hCNDHZJfGMy5szBoet3n96/Ws7QFv/p1Q/5fpH7 iIO01A+ScZ/smkOB8h9PIpgUXUEYmA92kt6Hq1vnGOwgpS+npc1BLR5a6WrSo5cUnGLA C6jA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20130820; h=x-gm-message-state:date:from:to:cc:subject:message-id:references :mime-version:content-disposition:in-reply-to:user-agent; bh=WjuQ6uvglX+BhkgNWK/xcfqgZsP6l/4QSb/FmG8wG/A=; b=ajiKoEEUoR2q/McrSMKT/VDfIn1xa/GieqN/2UYQXITmbg2rdCG6YvSFHciTkv9Gku L1n2aIfFj/YelEvYiRSbBN/bHnlB2ldatxTKhW0wAOsSmyiDWdSYVa5WZlRT6mtYDbGG Uj4aPYPQvSgGVhdyFwYfTrrW/ZBAcipBOAzveE0RJrBWgVFg5QB/d7Tef8pO/J4VLwJm B9yo6Xc+O9qo8FNpK0kDqM9LRcJmiY6POlVrAF3hMjSfrwysWe0ovDZxYMisA0n2x9WI oHNsoZIE2bF2Q43YJ/kyI05HGkd5F1XzMIkpTZFaaQpa0gQAHuW7aUPJBZHkh0/ClVsN 8Gkg== X-Gm-Message-State: AE9vXwMvlcFq6onnXn5GKzd/IhRbHwGCKDUZlLHUKSP41VXlrCPMtU9RzQAaJ5Nxru28wg== X-Received: by 10.194.156.226 with SMTP id wh2mr5220580wjb.150.1473088504765; Mon, 05 Sep 2016 08:15:04 -0700 (PDT) Received: from localhost (port-17845.pppoe.wtnet.de. [46.59.131.81]) by smtp.gmail.com with ESMTPSA id jd4sm28240834wjb.6.2016.09.05.08.15.03 (version=TLS1_2 cipher=ECDHE-RSA-CHACHA20-POLY1305 bits=256/256); Mon, 05 Sep 2016 08:15:03 -0700 (PDT) Date: Mon, 5 Sep 2016 17:15:02 +0200 From: Thierry Reding To: David Hsu Cc: gregkh@linuxfoundation.org, sspatil@google.com, linux-pwm@vger.kernel.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v2] pwm: Create device class for pwm channels Message-ID: <20160905151502.GS31424@ulmo.ba.sec> References: <1468880090-46076-1-git-send-email-davidhsu@google.com> <20160905144139.GQ31424@ulmo.ba.sec> MIME-Version: 1.0 Content-Disposition: inline In-Reply-To: <20160905144139.GQ31424@ulmo.ba.sec> User-Agent: Mutt/1.7.0 (2016-08-17) Sender: linux-pwm-owner@vger.kernel.org Precedence: bulk List-ID: X-Mailing-List: linux-pwm@vger.kernel.org On Mon, Sep 05, 2016 at 04:41:39PM +0200, Thierry Reding wrote: > On Mon, Jul 18, 2016 at 03:14:50PM -0700, David Hsu wrote: [...] > > diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c [...] > > @@ -248,9 +254,11 @@ static int pwm_export_child(struct device *parent, struct pwm_device *pwm) > > > > export->child.release = pwm_export_release; > > export->child.parent = parent; > > + export->child.type = &pwm_channel_type; > > export->child.devt = MKDEV(0, 0); > > + export->child.class = &pwm_class; > > This particular change isn't going to work, unfortunately. Children of a > PWM chip, i.e. PWM devices (or channels) are not the same as chips. The > above, however, will cause the attributes associated with a PWM chip to > be associated with each PWM device as well. For example it will cause a > PWM device to expose an "export" attribute in userspace. Writing to that > file will give you a nice oops, along these lines: > > [ 89.631855] Unable to handle kernel NULL pointer dereference at virtual address 00000014 > [ 89.640146] pgd = c2b16700 > [ 89.642879] [00000014] *pgd=82b74003, *pmd=f4dd8003 > [ 89.647830] Internal error: Oops: 207 [#1] PREEMPT SMP ARM > [ 89.653330] Modules linked in: nouveau tegra_drm ttm drm_kms_helper cfbfillrect syscopyarea cfbimgblt sysfillrect sysimgblt fb_sys_fops cfbcopyarea drm > [ 89.667194] CPU: 3 PID: 284 Comm: sh Not tainted 4.8.0-rc3-next-20160825-00026-g7065511b7003-dirty #82 > [ 89.676507] Hardware name: NVIDIA Tegra SoC (Flattened Device Tree) > [ 89.682787] task: c2b39500 task.stack: c3034000 > [ 89.687327] PC is at export_store+0x34/0x170 > [ 89.691598] LR is at _kstrtoull+0x2c/0x70 > [ 89.695607] pc : [] lr : [] psr: 60000013 > [ 89.695607] sp : c3035ea0 ip : c04b1c7c fp : bec8eb0c > [ 89.707068] r10: ee1e9b8c r9 : c3035f80 r8 : 00000002 > [ 89.712287] r7 : c2b70800 r6 : 00000000 r5 : ee1e9b80 r4 : 00000000 > [ 89.718806] r3 : 00000000 r2 : 00000000 r1 : 00000000 r0 : 00000000 > [ 89.725327] Flags: nZCv IRQs on FIQs on Mode SVC_32 ISA ARM Segment user > [ 89.732453] Control: 30c5387d Table: 82b16700 DAC: 55555555 > [ 89.738193] Process sh (pid: 284, stack limit = 0xc3034210) > [ 89.743758] Stack: (0xc3035ea0 to 0xc3036000) > [ 89.748116] 5ea0: ee1e9b8c 00000000 0014e408 00000002 ee1e9b80 00000000 00000000 ee19f180 > [ 89.756286] 5ec0: c3035f80 c035fc98 00000000 00000000 c3070180 c035fbd8 0014e408 c3035f80 > [ 89.764456] 5ee0: 00000000 00000002 00000000 c030167c 032cf000 c109bf44 c109bf44 c02f88b0 > [ 89.772625] 5f00: ee8270c0 c02fac24 00000001 c3035f10 ef05c9e0 c0300fd8 c0327c08 c2b7b000 > [ 89.780794] 5f20: 0000000a c2b7e000 c3184ec0 0000000a 00000400 c2b7e040 00000000 c3070180 > [ 89.788964] 5f40: 00000002 0014e408 c3035f80 00000000 00000002 c0302464 00000001 0000000a > [ 89.797132] 5f60: c2d076c0 c3070180 c3070180 00000000 00000000 0014e408 00000002 c03031b0 > [ 89.805302] 5f80: 00000000 00000000 00000014 00000002 0014e408 b6e74d50 00000004 c02075e4 > [ 89.813470] 5fa0: c3034000 c0207440 00000002 0014e408 00000001 0014e408 00000002 00000000 > [ 89.821638] 5fc0: 00000002 0014e408 b6e74d50 00000004 00000002 00000004 b6f0e000 bec8eb0c > [ 89.829808] 5fe0: 00000000 bec8ea64 b6d9ffa4 b6df970c 60000010 00000001 0b0a0908 0f0e0d0c > [ 89.837996] [] (export_store) from [] (kernfs_fop_write+0xc0/0x1cc) > [ 89.846005] [] (kernfs_fop_write) from [] (__vfs_write+0x1c/0x114) > [ 89.853940] [] (__vfs_write) from [] (vfs_write+0xa4/0x168) > [ 89.861252] [] (vfs_write) from [] (SyS_write+0x3c/0x90) > [ 89.868304] [] (SyS_write) from [] (ret_fast_syscall+0x0/0x34) > [ 89.875883] Code: ebff6164 e2506000 ba000039 e59d1004 (e5943014) > [ 89.882225] ---[ end trace 716eda7e65a4136a ]--- > > Given that we need a class associated with a device in order for it to > generate uevents (why is that so, by the way?), I think we'd need to add > a separate class implementation for PWM devices. That has the downside > of adding yet another subdirectory to /sys/class, but I can't really > think of another way of achieving what you need here. > > Greg, any ideas? *sigh*, nevermind. I realized too late that I hadn't fully applied your patch and the change to use device_create_with_groups() actually gets rid of this. Unfortunately.... > > index 01695d4..cb2b376 100644 > > --- a/drivers/pwm/sysfs.c > > +++ b/drivers/pwm/sysfs.c > > @@ -23,6 +23,8 @@ > > #include > > #include > > > > +static struct class pwm_class; > > + > > struct pwm_export { > > struct device child; > > struct pwm_device *pwm; > > @@ -222,6 +224,10 @@ static struct attribute *pwm_attrs[] = { > > }; > > ATTRIBUTE_GROUPS(pwm); > > > > +static const struct device_type pwm_channel_type = { > > + .name = "pwm_channel", > > +}; > > In order to do some tracing, I ended up implementing the ->uevent() > callback of struct device_type. What I noticed when exporting a PWM is > that that ->uevent() gets called recursively and the operation never > finishes. I have no idea why that happens, though. ... that's still there. However the output is now somewhat cleaner and the ->uevent() callback doesn't seem to be called recursively but rather repeatedly (until I unexport the PWM again). Let me investigate a little more where this is coming from. For reference, I have the below on top of your patch. Thierry --- >8 --- diff --git a/drivers/pwm/sysfs.c b/drivers/pwm/sysfs.c index 2ab5ed61d496..45e56a04f091 100644 --- a/drivers/pwm/sysfs.c +++ b/drivers/pwm/sysfs.c @@ -241,8 +241,17 @@ static struct attribute *pwm_attrs[] = { }; ATTRIBUTE_GROUPS(pwm); +static int pwm_uevent(struct device *dev, struct kobj_uevent_env *env) +{ + int err = 0; + dev_info(dev, "> %s(dev=%p, env=%p)\n", __func__, dev, env); + dev_info(dev, "< %s() = %d\n", __func__, err); + return err; +} + static const struct device_type pwm_channel_type = { - .name = "pwm_channel", + .name = "pwm_channel", + .uevent = pwm_uevent, }; static void pwm_export_release(struct device *child)