Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753510AbbFHUsH (ORCPT ); Mon, 8 Jun 2015 16:48:07 -0400 Received: from mail-lb0-f173.google.com ([209.85.217.173]:36484 "EHLO mail-lb0-f173.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753418AbbFHUsB (ORCPT ); Mon, 8 Jun 2015 16:48:01 -0400 MIME-Version: 1.0 In-Reply-To: References: <1433686811-12303-1-git-send-email-grant.likely@linaro.org> <1433686811-12303-3-git-send-email-grant.likely@linaro.org> <20150608184713.446B7C406AA@trevor.secretlab.ca> From: Ricardo Ribalda Delgado Date: Mon, 8 Jun 2015 22:47:38 +0200 Message-ID: Subject: Re: [PATCH 2/2] drivercore: Fix unregistration path of platform devices To: Grant Likely Cc: Rob Herring , Bjorn Helgaas , "devicetree@vger.kernel.org" , LKML , Pantelis Antoniou , Wolfram Sang , Rob Herring , Greg Kroah-Hartman Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 5212 Lines: 154 Hello Regarding the two problems 1) The immediate bug fix for dt unload I agree that we should use the simplest possible patch for backporting, but I believe that Grant patch does not differ too much from https://git.kernel.org/cgit/linux/kernel/git/gregkh/driver-core.git/commit/?h=driver-core-next&id=e50e69d1ac4232af0b6890f16929bf5ceee81538 which is already in driver-core-next and throws a nice warning message for debugging. I believe that this is the patch that should be backported. 2) Not adding resources: Until we found a solution for the platforms that are broken we only need to revert https://git.kernel.org/cgit/linux/kernel/git/gregkh/driver-core.git/commit/?h=driver-core-next&id=b6d2233f2916fa9338786aeab2e936c5a07e4d0c We get a lot of code cleanout (already reviewed) by doing this. I think reverting the whole series is not the best solution specially being a v5 :) Anyway whatever we decide I have some hardware where I can run tests if needed Regards and sorry for the flood! On Mon, Jun 8, 2015 at 10:09 PM, Ricardo Ribalda Delgado wrote: > Hello Grant > > > On Mon, Jun 8, 2015 at 8:47 PM, Grant Likely wrote: >> Hi Ricardo, >> >> Comments below... >> >> On Sun, 7 Jun 2015 20:13:15 +0200 >> , Ricardo Ribalda Delgado >> wrote: >>> Hello Grant >>> >>> I would ask you to go through all the discussion related to this bug. >>> Here is a summary (please anyone involved correct me if I am wrong) >>> >>> 1) I send a patch to fix the oops if release resource is executed with >>> a resource without parent >>> 2) Bjorn says that we should fix the issue of the problem, which he >>> pointed out being that we use platform_device_del() after using >>> of_device_add() >> >> Bjorn's comments on v3 of your patchset were correct. The proposed bug >> fix hacked the __release_resource function directly, when the bug is in >> the platform_bus_type code. >> > > The bug is not in the platform subsystem but in the of subsystem. Your > patch fixes it in the platform subsystem, so it is as bad as fixing it > directly on the resource interface. > > >>> 3) I resend a patchset to use platform_devide_add() >>> 4) 3 series of cleanouts after the help from Rob and Bjorn >>> 5) Greg adds the series (v5) to his device core tree >> >> The series is still wrong. >> >> Greg, please drop Ricardo's series. It isn't correct and it will cause >> breakage. > > The series can be kept, only > > patch "of/platform: Use platform_device interface" > > needs to be reverted. > >> >> There are two issues that need to be delt with: >> >> First, there is the immediate bug fix which should go to Linus before >> v4.1. I believe my patch handles it correctly. I've included a test >> case, but I would like to have acks from Rob and Pantelis before merging >> it. Ricardo's v5 patch 2/4 comes close to solving it, but it still >> doesn't make the unregister path symmetric with the register path. > > Could you please be more specific. what is not symmetric after > applying the patchset? > > >> Second, there is the issue of making devicetree platform_devices request >> resources. That's harder, and we are *NOT* ready to merge anything. Nor >> is it a time critical issue. >> >>> 6) You complaint that that series can break miss behaved platforms >> >> Yes, because it will. >> >>> 7) I send a couple of patches that fix your problem and leaves the >>> window open to blacklist the platforms that miss behave. >> >> I've replied to that series. It isn't a good solution either. > > I have also replied, please provide a testcase and we will figure it > if it is not handled properly. So far it works fine on my tests. > >>> >>> now you send a patch that takes us to back to step 1), and adds some >>> code that is already merged into gregk's >>> https://git.kernel.org/cgit/linux/kernel/git/gregkh/driver-core.git/tree/drivers/base/platform.c?h=driver-core-testing#n314 >> >> My patch is different. In v3 __release_resource was hacked directly. By >> v5 you were fixing platform_device_{add,del}, which is the right thing, >> but still isn't symmetric. My patch I think handles the bug fix >> correctly. > > There is no need to apply your patch, that behaviour is already > impletented in my patchset. If we want to pospone the non registry of > resources on of devices we just need to revert > > "of/platform: Use platform_device interface" > > I believe reverting 1 patch is patch is better than reverting 4 > reviewed patches and applying a new one. > >> >>> Wouldn't you agree that it will be a better solution to give your >>> feedback regarding https://lkml.org/lkml/2015/6/5/246 and fix this >>> issue together? >> >> That I've done. I'm not happy with it. Sorry. > > No worries :), but we need to find another sollution. And if we can > remove all the duplicated code in /of we will have much less bugs in > the future. > > > Regards -- Ricardo Ribalda -- To unsubscribe from this list: send the line "unsubscribe linux-kernel" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html Please read the FAQ at http://www.tux.org/lkml/