On Tue, Sep 06, 2016 at 10:36:49AM +0200, Neil Armstrong wrote:
> Hi Thierry,
>
> On 09/05/2016 11:00 AM, Thierry Reding wrote:
> > On Mon, Aug 22, 2016 at 05:36:30PM +0200, Neil Armstrong wrote:
> >> Add support for the PWM controller found in the Amlogic SoCs.
> >> This driver supports the Meson8b and GXBB SoCs.
> >>
> >> Signed-off-by: Neil Armstrong <narmstr...@baylibre.com>
> >> ---
> >> drivers/pwm/Kconfig | 9 +
> >> drivers/pwm/Makefile | 1 +
> >> drivers/pwm/pwm-meson.c | 528
> >> ++++++++++++++++++++++++++++++++++++++++++++++++
> >> 3 files changed, 538 insertions(+)
> >> create mode 100644 drivers/pwm/pwm-meson.c
> >
> > Hi Neil,
> >
> > sorry for taking so long to review this. I had actually started to write
> > a review email since I had noticed a couple of slight oddities about the
> > driver structure (primarily this was about how channel-specific data was
> > split between struct meson_pwm_channel and struct meson_pwm_chip), but I
> > ended up making some changes to the driver in order to see what my
> > suggestions would look like, and if they would indeed improve things.
> > But once I had done that, I thought it a bit pointless to make that into
> > review comments and decided to just push what I had done and ask you to
> > take a look, and if you had no objections to the changes take the driver
> > for a spin to see if it still worked as expected.
>
> We re-run our tests and I found 2 bugs, the first one is in
> meson_pwm_enable(),
> only the channel A was setup, the fix is :
>
> static void meson_pwm_enable(...)
> - u32 value, clk_shift, clk_enable, enable;
> + u32 reg, value, clk_shift, clk_enable, enable;
>
> switch (id) {
> case 0:
> [...]
> + reg = REG_PWM_A;
> break;
> case 1:
> [...]
> + reg = REG_PWM_B;
> break;
> }
> [...]
> - writel(value, meson->base + REG_PWM_A);
> + writel(value, meson->base + reg);
Ah indeed. Good catch.
>
> 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():
>
> static int meson_pwm_init_channels(..., struct meson_pwm_channel *channels)
> {
> + struct meson_pwm_channel *channels;
> [...]
> - for (i = 0; i < meson->chip.npwm; i++) {
> - struct pwm_device *pwm = &meson->chip.pwms[i];
> - struct meson_pwm_channel *channel;
> -
> - channel = devm_kzalloc(dev, sizeof(*channel), GFP_KERNEL);
> - if (!channel)
> - return -ENOMEM;
> + if (!channels)
> + return -EINVAL;
>
> + for (i = 0; i < meson->chip.npwm; i++) {
> [...]
> + memset(&channels[i], 0, sizeof(struct meson_pwm_channel));
> [...]
> //Rename "channel->" into "channels[i]."//
> [...]
> - pwm_set_chip_data(pwm, channel);
> }
>
> return 0;
> }
>
> +static void meson_pwm_add_channels_data(struct meson_pwm *meson,
> + struct meson_pwm_channel *channels)
> +{
> + unsigned int i;
> +
> + for (i = 0; i < meson->chip.npwm; i++)
> + pwm_set_chip_data(&meson->chip.pwms[i], &channels[i]);
> +}
>
> static int meson_pwm_probe(struct platform_device *pdev)
> {
> + struct meson_pwm_channel *channels;
> [...]
> - err = meson_pwm_init_channels(meson);
> - if (err < 0)
> - return err;
> -
> meson->chip.dev = &pdev->dev;
> [...]
> meson->chip.of_pwm_n_cells = 3;
>
> + channels = devm_kmalloc_array(&pdev->dev, 2, sizeof(*meson),
> + GFP_KERNEL);
> + if (!channels)
> + return -ENOMEM;
> +
> + err = meson_pwm_init_channels(meson, channels);
> + if (err < 0)
> + return err;
> +
> err = pwmchip_add(&meson->chip);
> [...]
> + meson_pwm_add_channels_data(meson, channels);
> +
> platform_set_drvdata(pdev, meson);
>
> return 0;
> }
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.
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. =)
Thierry
signature.asc
Description: PGP signature
