Re: mainline/master boot bisection: v4.15-rc3 on peach-pi #3228-staging

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



[adding Marek and Shuah to cc list]

On Mon, Dec 11, 2017 at 6:05 PM, Daniel Vetter <daniel.vetter@xxxxxxxx> wrote:
> On Mon, Dec 11, 2017 at 11:30 AM, Guillaume Tucker
> <guillaume.tucker@xxxxxxxxxxxxx> wrote:
>> Hi Daniel,
>>
>> Please see below, I've had several bisection results pointing at
>> that commit over the week-end on mainline but also on linux-next
>> and net-next.  While the peach-pi is a bit flaky at the moment
>> and is likely to have more than one issue, it does seem like this
>> commit is causing some well reproducible kernel hang.
>>
>> Here's a re-run with v4.15-rc3 showing the issue:
>>
>>   https://lava.collabora.co.uk/scheduler/job/1018478
>>
>> and here's another one with the change mentioned below reverted:
>>
>>   https://lava.collabora.co.uk/scheduler/job/1018479
>>
>> They both show a warning about "unbalanced disables for lcd_vdd",
>> I don't know if this is related as I haven't investigated any
>> further.  It does appear to reliably hang with v4.15-rc3 and
>> boot most of the time with the commit reverted though.
>>
>> The automated kernelci.org bisection is still an experimental
>> tool and it may well be a false positive, so please take this
>> result with a pinch of salt...
>
> The patch just very minimal moves the connector cleanup around (so
> timing change), but except when you unload a driver (or maybe that
> funny EPROBE_DEFER stuff) it shouldn't matter. So if you don't have
> more info than "seems to hang a bit more" I have no idea what's wrong.
> The patch itself should work, at least it survived quite some serious
> testing we do on everything.
> -Daniel
>

Marek was pointing to a different culprit [0] in this [1] thread. I
see that both commits made it to v4.15-rc3, which is the first version
where boot fails. So maybe is a combination of both? Or rather
reverting one patch masks the error in the other.

I've access to the machine but unfortunately not a lot of time to dig
on this, I could try to do it in the weekend though.

[0]: https://patchwork.kernel.org/patch/10067711/
[1]: https://www.spinics.net/lists/arm-kernel/msg622152.html

Best regards,
Javier

>> Hope this helps!
>>
>> Best wishes,
>> Guillaume
>>
>>
>> -------- Forwarded Message --------
>> Subject: mainline/master boot bisection: v4.15-rc3 on peach-pi #3228-staging
>> Date: Mon, 11 Dec 2017 08:25:55 +0000 (UTC)
>> From: kernelci.org bot <bot@xxxxxxxxxxxx>
>> To: guillaume.tucker@xxxxxxxxxxxxx
>>
>> Bisection result for mainline/master (v4.15-rc3) on peach-pi
>>
>> Good known revision:
>>
>>     c6b3e96 Merge branch 'for-linus' of
>> git://git.kernel.org/pub/scm/linux/kernel/git/s390/linux
>>
>> Bad known revision:
>>
>>     50c4c4e Linux 4.15-rc3
>>
>> Extra parameters:
>>
>>     Tree:      mainline
>>     Branch:    master
>>     Target:    peach-pi
>>     Lab:       lab-collabora
>>     Defconfig: exynos_defconfig
>>     Plan:      boot
>>
>>
>> Breaking commit found:
>>
>> -------------------------------------------------------------------------------
>> commit a703c55004e1c5076d57e43771b3e11117796ea0
>> Author: Daniel Vetter <daniel.vetter@xxxxxxxx>
>> Date:   Mon Dec 4 21:48:18 2017 +0100
>>
>>     drm: safely free connectors from connector_iter
>>         In
>>         commit 613051dac40da1751ab269572766d3348d45a197
>>     Author: Daniel Vetter <daniel.vetter@xxxxxxxx>
>>     Date:   Wed Dec 14 00:08:06 2016 +0100
>>             drm: locking&new iterators for connector_list
>>         we've went to extreme lengths to make sure connector iterations
>> works
>>     in any context, without introducing any additional locking context.
>>     This worked, except for a small fumble in the implementation:
>>         When we actually race with a concurrent connector unplug event, and
>>     our temporary connector reference turns out to be the final one, then
>>     everything breaks: We call the connector release function from
>>     whatever context we happen to be in, which can be an irq/atomic
>>     context. And connector freeing grabs all kinds of locks and stuff.
>>         Fix this by creating a specially safe put function for
>> connetor_iter,
>>     which (in this rare case) punts the cleanup to a worker.
>>         Reported-by: Ben Widawsky <ben@xxxxxxxxxxxx>
>>     Cc: Ben Widawsky <ben@xxxxxxxxxxxx>
>>     Fixes: 613051dac40d ("drm: locking&new iterators for connector_list")
>>     Cc: Dave Airlie <airlied@xxxxxxxxx>
>>     Cc: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
>>     Cc: Sean Paul <seanpaul@xxxxxxxxxxxx>
>>     Cc: <stable@xxxxxxxxxxxxxxx> # v4.11+
>>     Reviewed-by: Dave Airlie <airlied@xxxxxxxxx>
>>     Signed-off-by: Daniel Vetter <daniel.vetter@xxxxxxxxx>
>>     Link:
>> https://patchwork.freedesktop.org/patch/msgid/20171204204818.24745-1-daniel.vetter@xxxxxxxx
>>
>> diff --git a/drivers/gpu/drm/drm_connector.c
>> b/drivers/gpu/drm/drm_connector.c
>> index 25f4b2e..4820141 100644
>> --- a/drivers/gpu/drm/drm_connector.c
>> +++ b/drivers/gpu/drm/drm_connector.c
>> @@ -152,6 +152,16 @@ static void drm_connector_free(struct kref *kref)
>>         connector->funcs->destroy(connector);
>>  }
>>  +static void drm_connector_free_work_fn(struct work_struct *work)
>> +{
>> +       struct drm_connector *connector =
>> +               container_of(work, struct drm_connector, free_work);
>> +       struct drm_device *dev = connector->dev;
>> +
>> +       drm_mode_object_unregister(dev, &connector->base);
>> +       connector->funcs->destroy(connector);
>> +}
>> +
>>  /**
>>   * drm_connector_init - Init a preallocated connector
>>   * @dev: DRM device
>> @@ -181,6 +191,8 @@ int drm_connector_init(struct drm_device *dev,
>>         if (ret)
>>                 return ret;
>>  +      INIT_WORK(&connector->free_work, drm_connector_free_work_fn);
>> +
>>         connector->base.properties = &connector->properties;
>>         connector->dev = dev;
>>         connector->funcs = funcs;
>> @@ -529,6 +541,18 @@ void drm_connector_list_iter_begin(struct drm_device
>> *dev,
>>  }
>>  EXPORT_SYMBOL(drm_connector_list_iter_begin);
>>  +/*
>> + * Extra-safe connector put function that works in any context. Should only
>> be
>> + * used from the connector_iter functions, where we never really expect to
>> + * actually release the connector when dropping our final reference.
>> + */
>> +static void
>> +drm_connector_put_safe(struct drm_connector *conn)
>> +{
>> +       if (refcount_dec_and_test(&conn->base.refcount.refcount))
>> +               schedule_work(&conn->free_work);
>> +}
>> +
>>  /**
>>   * drm_connector_list_iter_next - return next connector
>>   * @iter: connectr_list iterator
>> @@ -561,7 +585,7 @@ drm_connector_list_iter_next(struct
>> drm_connector_list_iter *iter)
>>         spin_unlock_irqrestore(&config->connector_list_lock, flags);
>>         if (old_conn)
>> -               drm_connector_put(old_conn);
>> +               drm_connector_put_safe(old_conn);
>>         return iter->conn;
>>  }
>> @@ -580,7 +604,7 @@ void drm_connector_list_iter_end(struct
>> drm_connector_list_iter *iter)
>>  {
>>         iter->dev = NULL;
>>         if (iter->conn)
>> -               drm_connector_put(iter->conn);
>> +               drm_connector_put_safe(iter->conn);
>>         lock_release(&connector_list_iter_dep_map, 0, _RET_IP_);
>>  }
>>  EXPORT_SYMBOL(drm_connector_list_iter_end);
>> diff --git a/drivers/gpu/drm/drm_mode_config.c
>> b/drivers/gpu/drm/drm_mode_config.c
>> index cda8bfa..cc78b3d 100644
>> --- a/drivers/gpu/drm/drm_mode_config.c
>> +++ b/drivers/gpu/drm/drm_mode_config.c
>> @@ -431,6 +431,8 @@ void drm_mode_config_cleanup(struct drm_device *dev)
>>                 drm_connector_put(connector);
>>         }
>>         drm_connector_list_iter_end(&conn_iter);
>> +       /* connector_iter drops references in a work item. */
>> +       flush_scheduled_work();
>>         if (WARN_ON(!list_empty(&dev->mode_config.connector_list))) {
>>                 drm_connector_list_iter_begin(dev, &conn_iter);
>>                 drm_for_each_connector_iter(connector, &conn_iter)
>> diff --git a/include/drm/drm_connector.h b/include/drm/drm_connector.h
>> index df9807a..a4649c5 100644
>> --- a/include/drm/drm_connector.h
>> +++ b/include/drm/drm_connector.h
>> @@ -916,6 +916,14 @@ struct drm_connector {
>>         uint8_t num_h_tile, num_v_tile;
>>         uint8_t tile_h_loc, tile_v_loc;
>>         uint16_t tile_h_size, tile_v_size;
>> +
>> +       /**
>> +        * @free_work:
>> +        *
>> +        * Work used only by &drm_connector_iter to be able to clean up a
>> +        * connector from any context.
>> +        */
>> +       struct work_struct free_work;
>>  };
>>   #define obj_to_connector(x) container_of(x, struct drm_connector, base)
>> -------------------------------------------------------------------------------
>>
>>
>> Git bisection log:
>>
>> -------------------------------------------------------------------------------
>> git bisect start
>> # good: [c6b3e9693f8a32ba3b07e2f2723886ea2aff4e94] Merge branch 'for-linus'
>> of git://git.kernel.org/pub/scm/linux/kernel/git/s390/linux
>> git bisect good c6b3e9693f8a32ba3b07e2f2723886ea2aff4e94
>> # bad: [50c4c4e268a2d7a3e58ebb698ac74da0de40ae36] Linux 4.15-rc3
>> git bisect bad 50c4c4e268a2d7a3e58ebb698ac74da0de40ae36
>> # bad: [e9ef1fe312b533592e39cddc1327463c30b0ed8d] Merge
>> git://git.kernel.org/pub/scm/linux/kernel/git/davem/net
>> git bisect bad e9ef1fe312b533592e39cddc1327463c30b0ed8d
>> # bad: [77071bc6c472bb0b36818f3e9595114cdf98c86d] Merge tag 'media/v4.15-2'
>> of git://git.kernel.org/pub/scm/linux/kernel/git/mchehab/linux-media
>> git bisect bad 77071bc6c472bb0b36818f3e9595114cdf98c86d
>> # bad: [4066aa72f9f2886105c6f747d7f9bd4f14f53c12] Merge tag
>> 'drm-fixes-for-v4.15-rc3' of git://people.freedesktop.org/~airlied/linux
>> git bisect bad 4066aa72f9f2886105c6f747d7f9bd4f14f53c12
>> # bad: [96980844bb4b74d2e7ce93d907670658e39a3992] Merge tag
>> 'drm-intel-fixes-2017-12-07' of git://anongit.freedesktop.org/drm/drm-intel
>> into drm-fixes
>> git bisect bad 96980844bb4b74d2e7ce93d907670658e39a3992
>> # bad: [120a264f9c2782682027d931d83dcbd22e01da80] drm/exynos: gem: Drop
>> NONCONTIG flag for buffers allocated without IOMMU
>> git bisect bad 120a264f9c2782682027d931d83dcbd22e01da80
>> # good: [2bf257d662509553ae226239e7dc1c3d00636ca6] drm/ttm: roundup the
>> shrink request to prevent skip huge pool
>> git bisect good 2bf257d662509553ae226239e7dc1c3d00636ca6
>> # good: [db8f884ca7fe6af64d443d1510464efe23826131] Merge branch
>> 'drm-fixes-4.15' of git://people.freedesktop.org/~agd5f/linux into drm-fixes
>> git bisect good db8f884ca7fe6af64d443d1510464efe23826131
>> # bad: [bd3a3a2e92624942a143e485c83e641b2492d828] Merge tag
>> 'drm-misc-fixes-2017-12-06' of git://anongit.freedesktop.org/drm/drm-misc
>> into drm-fixes
>> git bisect bad bd3a3a2e92624942a143e485c83e641b2492d828
>> # bad: [a703c55004e1c5076d57e43771b3e11117796ea0] drm: safely free
>> connectors from connector_iter
>> git bisect bad a703c55004e1c5076d57e43771b3e11117796ea0
>> # first bad commit: [a703c55004e1c5076d57e43771b3e11117796ea0] drm: safely
>> free connectors from connector_iter
>> -------------------------------------------------------------------------------
>
>
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
> --
> To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
> the body of a message to majordomo@xxxxxxxxxxxxxxx
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe linux-samsung-soc" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html



[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  Powered by Linux