From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756020AbcIFJPz (ORCPT ); Tue, 6 Sep 2016 05:15:55 -0400 Received: from mail-wm0-f52.google.com ([74.125.82.52]:36823 "EHLO mail-wm0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934275AbcIFJOh (ORCPT ); Tue, 6 Sep 2016 05:14:37 -0400 Subject: Re: [PATCH v3 1/4] pwm: Add support for Meson PWM Controller To: Thierry Reding References: <1471880193-21879-1-git-send-email-narmstrong@baylibre.com> <1471880193-21879-2-git-send-email-narmstrong@baylibre.com> <20160905090052.GG3532@ulmo.ba.sec> <20160906090749.GA15644@ulmo.ba.sec> Cc: carlo@caione.org, khilman@baylibre.com, linux-pwm@vger.kernel.org, linux-arm-kernel@lists.infradead.org, linux-amlogic@lists.infradead.org, linux-kernel@vger.kernel.org From: Neil Armstrong Organization: Baylibre Message-ID: Date: Tue, 6 Sep 2016 11:14:45 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:45.0) Gecko/20100101 Thunderbird/45.2.0 MIME-Version: 1.0 In-Reply-To: <20160906090749.GA15644@ulmo.ba.sec> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 09/06/2016 11:07 AM, Thierry Reding wrote: > On Tue, Sep 06, 2016 at 10:36:49AM +0200, Neil Armstrong wrote: >> Hi Thierry, >> [...] > >> >> The second bug is in probe(), I understand the point to allocate >> dynamically the channels and attach them to each pwm chip, but when >> calling meson_pwm_init_channels() we get an OOPS because >> meson->chip.pwms[i] are allocated in pwmchip_add(). Moving >> meson_pwm_init_channels() would fix this, but in case of a clk >> PROBE_DEFER, we would need to remove back the pwmchip, which is a >> quite a bad design decision.... > > Ah yes... that one again. I remember running into that a while ago with > some other driver. To be honest, I think that's a short-coming of the > PWM subsystem and the fix would be for PWM chip registration to be split > into two parts: pwm_chip_init() and pwm_chip_add(). That way, a chip > would be initialized using pwm_chip_init() where the pwms array would be > allocated, and pwm_chip_add() would register the chip with the system. > > Currently a few drivers might be vulnerable to a race condition between > registration and implementation (i.e. PWM channels aren't fully set up > when they are exposed to users and sysfs). > >> The smartest fix I found was to allocate channels in probe, init them >> them attach them after pwmchip_add(): >> [...] > > That's the race I was talking about above. I suppose it's not too big an > issue since other drivers seem to manage, so I'm going to merge your > fixed driver. ok thanks ! > > Unless you feel like taking a stab at the pwm_chip_init()/pwm_chip_add() > split, in which case your driver would be the first to be race-free. =) Having he driver upstream is a priority, but having it completely race-free would be great! I'll be happy to collaborate to a race-free pwmchip probe somehow ! But there is still a glitch, when pwmadd_chip() returns, pwm_get_chip_data(pwm) will still return crap until meson_pwm_add_channels_data() is called in the following instructions... The pwm_chip_init()/pwm_chip_add() would be the only solution ! > Thierry > Thanks, Neil