Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug

2015-11-04 Thread Ni, Ruiyu 
The latest platform recovery patch I sent out was only two commits in local.
I split the two to eleven to avoid you complain again:)

Regards,
Ray

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Leif 
Lindholm
Sent: Thursday, November 5, 2015 1:13 AM
To: Ni, Ruiyu <ruiyu...@intel.com>
Cc: Kinney, Michael D <michael.d.kin...@intel.com>; Tian, Feng 
<feng.t...@intel.com>; edk2-devel@lists.01.org; Fan, Jeff <jeff@intel.com>
Subject: Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug

Hi Ray,

On Tue, Nov 03, 2015 at 02:27:29PM +, Ni, Ruiyu wrote:
> Leif,
> I saw your feedbacks as your opinion if you create patches. I don't
> think your opinion is the *only* right solution.

Oh, certainly. I was only expecting some sort of response before the
patches were committed again.

> I can also split 3/4 in your patch serials to two patches: one is to
> just change DumpBridgeResource(), the other is for the remaining
> changes.
> So I think my splitting way is good enough.

And I will always argue for splitting more :)

> OK I will give response to your questions firstly.

Thanks.

Regards,

Leif
 
> Regards,
> Ray
> 
> -Original Message-
> From: Leif Lindholm [mailto:leif.lindh...@linaro.org] 
> Sent: Tuesday, November 3, 2015 6:45 PM
> To: Ni, Ruiyu <ruiyu...@intel.com>
> Cc: Tian, Feng <feng.t...@intel.com>; Kinney, Michael D 
> <michael.d.kin...@intel.com>; edk2-devel@lists.01.org; Fan, Jeff 
> <jeff@intel.com>
> Subject: Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug
> 
> Hi guys,
> 
> I never saw a response to this feedback I sent on the revised patch
> set, but I see it has now been committed.
> Could you let me know why my feedback was ignored?
> 
> Regards,
> 
> Leif
> 
> On 30 October 2015 at 18:02, Leif Lindholm <leif.lindh...@linaro.org> wrote:
> > Hi Ray,
> >
> > Many thanks for this.
> >
> > On Fri, Oct 30, 2015 at 04:53:54AM +, Ni, Ruiyu wrote:
> >> Ok I will revert the two patches I committed (big patch + small
> >> patch). But I want to clarify one thing that not every big patch can
> >> be easily understood by just splitting to small patches.
> >
> > Certainly, but it is always helpful to split as much as is possible.
> > The shorter a patch is to review, the more likely reviewers are to
> > spot mistakes - especially when reviewing code they are not already
> > very familiar with.
> >
> > As an example, I have split your set up a little bit further by
> > breaking 2/3 into two separate patches.
> > https://git.linaro.org/people/leif.lindholm/edk2.git/shortlog/refs/heads/ray-pci
> >
> > I have also tried to remove modifications completely unrelated to this
> > fix - resulting in a difference between the tree with your current
> > patch set and my variant as included below. The changes in your set
> > are all valid changes, but they are not related to the fix.
> >
> > The method behind all of this madness is to make the output of "git
> > blame" as relevant as possible, as well as making automated "git
> > bisect" sessions for tracking down which specific change caused an
> > issue more useful.
> >
> > Best Regards,
> >
> > Leif
> >
> >
> > Diff between Ray's v2 and my version of it:
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c 
> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> > index 12647be..523c698 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> > @@ -281,13 +281,13 @@ DumpResourceMap (
> >IN UINTN ResourceCount
> >)
> >  {
> > -  EFI_STATUS   Status;
> > -  LIST_ENTRY   *Link;
> > -  PCI_IO_DEVICE*Device;
> > -  UINTNIndex;
> > -  CHAR16   *Str;
> > -  PCI_RESOURCE_NODE**ChildResources;
> > -  UINTNChildResourceCount;
> > +  EFI_STATUS   Status;^M
> > +  LIST_ENTRY   *Link;^M
> > +  PCI_IO_DEVICE*Device;^M
> > +  UINTNIndex;^M
> > +  CHAR16   *Str;^M
> > +  PCI_RESOURCE_NODE**ChildResources;^M
> > +  UINTNChildResourceCount;^M
> >
> >DEBUG ((EFI_D_INFO, "PciBus: Resource Map for "));
> >
> > @@ -976,7 +976,7 @@ PciScanBus (
> >UINT8 Device;
> >UINT8

Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug

2015-11-04 Thread Leif Lindholm 
Hi Ray,

On Tue, Nov 03, 2015 at 02:27:29PM +, Ni, Ruiyu wrote:
> Leif,
> I saw your feedbacks as your opinion if you create patches. I don't
> think your opinion is the *only* right solution.

Oh, certainly. I was only expecting some sort of response before the
patches were committed again.

> I can also split 3/4 in your patch serials to two patches: one is to
> just change DumpBridgeResource(), the other is for the remaining
> changes.
> So I think my splitting way is good enough.

And I will always argue for splitting more :)

> OK I will give response to your questions firstly.

Thanks.

Regards,

Leif
 
> Regards,
> Ray
> 
> -Original Message-
> From: Leif Lindholm [mailto:leif.lindh...@linaro.org] 
> Sent: Tuesday, November 3, 2015 6:45 PM
> To: Ni, Ruiyu <ruiyu...@intel.com>
> Cc: Tian, Feng <feng.t...@intel.com>; Kinney, Michael D 
> <michael.d.kin...@intel.com>; edk2-devel@lists.01.org; Fan, Jeff 
> <jeff....@intel.com>
> Subject: Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug
> 
> Hi guys,
> 
> I never saw a response to this feedback I sent on the revised patch
> set, but I see it has now been committed.
> Could you let me know why my feedback was ignored?
> 
> Regards,
> 
> Leif
> 
> On 30 October 2015 at 18:02, Leif Lindholm <leif.lindh...@linaro.org> wrote:
> > Hi Ray,
> >
> > Many thanks for this.
> >
> > On Fri, Oct 30, 2015 at 04:53:54AM +, Ni, Ruiyu wrote:
> >> Ok I will revert the two patches I committed (big patch + small
> >> patch). But I want to clarify one thing that not every big patch can
> >> be easily understood by just splitting to small patches.
> >
> > Certainly, but it is always helpful to split as much as is possible.
> > The shorter a patch is to review, the more likely reviewers are to
> > spot mistakes - especially when reviewing code they are not already
> > very familiar with.
> >
> > As an example, I have split your set up a little bit further by
> > breaking 2/3 into two separate patches.
> > https://git.linaro.org/people/leif.lindholm/edk2.git/shortlog/refs/heads/ray-pci
> >
> > I have also tried to remove modifications completely unrelated to this
> > fix - resulting in a difference between the tree with your current
> > patch set and my variant as included below. The changes in your set
> > are all valid changes, but they are not related to the fix.
> >
> > The method behind all of this madness is to make the output of "git
> > blame" as relevant as possible, as well as making automated "git
> > bisect" sessions for tracking down which specific change caused an
> > issue more useful.
> >
> > Best Regards,
> >
> > Leif
> >
> >
> > Diff between Ray's v2 and my version of it:
> >
> > diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c 
> > b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> > index 12647be..523c698 100644
> > --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> > +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> > @@ -281,13 +281,13 @@ DumpResourceMap (
> >IN UINTN ResourceCount
> >)
> >  {
> > -  EFI_STATUS   Status;
> > -  LIST_ENTRY   *Link;
> > -  PCI_IO_DEVICE*Device;
> > -  UINTNIndex;
> > -  CHAR16   *Str;
> > -  PCI_RESOURCE_NODE**ChildResources;
> > -  UINTNChildResourceCount;
> > +  EFI_STATUS   Status;^M
> > +  LIST_ENTRY   *Link;^M
> > +  PCI_IO_DEVICE*Device;^M
> > +  UINTNIndex;^M
> > +  CHAR16   *Str;^M
> > +  PCI_RESOURCE_NODE**ChildResources;^M
> > +  UINTNChildResourceCount;^M
> >
> >DEBUG ((EFI_D_INFO, "PciBus: Resource Map for "));
> >
> > @@ -976,7 +976,7 @@ PciScanBus (
> >UINT8 Device;
> >UINT8 Func;
> >UINT64Address;
> > -  UINT8 SecondBus;
> > +  UINTN SecondBus;^M
> >UINT8 PaddedSubBus;
> >UINT16Register;
> >UINTN HpIndex;
> > @@ -1211,7 +1211,7 @@ PciScanBus (
> >
> >Status = PciScanBus (
> >  PciDevice,
> > -SecondBus,
> > +(UINT8) (

Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug

2015-11-03 Thread Leif Lindholm 
rture & (Bridge->Alignment);
>
> @@ -346,6 +348,7 @@ CalculateResourceAperture (
>UINT64Aperture;
>LIST_ENTRY*CurrentLink;
>PCI_RESOURCE_NODE *Node;
> +^M
>UINT64PaddingAperture;
>UINT64Offset;
>
> @@ -419,7 +422,7 @@ CalculateResourceAperture (
>}
>
>//
> -  // Adjust the bridge's alignment to the first child's alignment
> +  // At last, adjust the bridge's alignment to the first child's
>// alignment^M
>    // if the bridge has at least one child
>    //
>CurrentLink = Bridge->ChildList.ForwardLink;
>
>
>>
>> Thanks,
>> Ray
>>
>> -Original Message-
>> From: Tian, Feng
>> Sent: Friday, October 30, 2015 9:57 AM
>> To: Leif Lindholm <leif.lindh...@linaro.org>
>> Cc: Ni, Ruiyu <ruiyu...@intel.com>; Kinney, Michael D 
>> <michael.d.kin...@intel.com>; edk2-devel@lists.01.org; Fan, Jeff 
>> <jeff@intel.com>; Tian, Feng <feng.t...@intel.com>
>> Subject: RE: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug
>>
>> Leif,
>>
>> Thanks for raising this issue.
>>
>> I agree with you that the patch should be split to small ones and make it 
>> more readable. I also agree we should give community more time to review 
>> those big patch before getting committed in.
>>
>> Ray, who is the module owner, will follow up your suggestions.
>>
>> Thanks
>> Feng
>>
>> -Original Message-
>> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Leif 
>> Lindholm
>> Sent: Thursday, October 29, 2015 19:55
>> To: Tian, Feng
>> Cc: Ni, Ruiyu; Kinney, Michael D; edk2-devel@lists.01.org; Fan, Jeff
>> Subject: Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug
>>
>> Hi Feng,
>>
>> On Thu, Oct 29, 2015 at 12:23:04AM +, Tian, Feng wrote:
>> > Ray has sent the patch for review, you may ignore it.
>> > (http://article.gmane.org/gmane.comp.bios.edk2.devel/3484)
>>
>> Yes, I see it has now been committed - which solves the known bug.
>>
>> But this still leaves us with a clearly not sufficiently reviewed/tested 
>> invasive patch in core PCI code, and a jumbled commit history.
>>
>> Since I have not received any response when asking previously, I will ask 
>> again: Can we please revert SVN r18658 and introduce this changeset in a 
>> more reviewable form, with proper review/testing?
>>
>> Regards,
>>
>> Leif
>>
>> > -Original Message-
>> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of
>> > Leif Lindholm
>> > Sent: Wednesday, October 28, 2015 22:51
>> > To: Tian, Feng
>> > Cc: Ni, Ruiyu; edk2-devel@lists.01.org; Fan, Jeff
>> > Subject: Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug
>> >
>> > Feng,
>> >
>> > Any update on the below?
>> > The hard crash bug is still in SVN r18690.
>> >
>> > Regards,
>> >
>> > Leif
>> >
>> > On Mon, Oct 26, 2015 at 03:35:15PM +, Leif Lindholm wrote:
>> > > Hi Ray,
>> > >
>> > > On Mon, Oct 26, 2015 at 03:17:22PM +, Ni, Ruiyu wrote:
>> > > > Can you add a null pointer check to
>> > > > PciIoDevice->ResourcePaddingDescriptors before calling
>> > > > DumpPpbPaddingResource? Does it help?
>> > >
>> > > It removes the delay and the crash - thanks!
>> > >
>> > > But it does nothing for the commit history ;)
>> > >
>> > > Regards,
>> > >
>> > > Leif
>> > >
>> > > > > 在 2015年10月26日，21:13，Leif Lindholm <leif.lindh...@linaro.org> 写道：
>> > > > >
>> > > > > Hi Ruiyu, Feng,
>> > > > >
>> > > > > I am currently tracking down an issue on (at least) one of my
>> > > > > platforms - that happens with this (now committed) patch, but
>> > > > > not without it.
>> > > > >
>> > > > > Symptoms are a _long_ delay, followed by an unaligned access in
>> > > > > (I
>> > > > > think) DumpPpbPaddingResource.
>> > > > >
>> > > > > Anyway, there could be other things playing in here - I'm
>> > > > > testing a new card in a platform I haven't previously tested cards.
>> > > > >
>> > > > > But this is one

Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug

2015-11-03 Thread Ni, Ruiyu 
Leif,
I saw your feedbacks as your opinion if you create patches. I don't think your 
opinion is the *only* right solution.
I can also split 3/4 in your patch serials to two patches: one is to just 
change DumpBridgeResource(), the other is for the remaining changes.
So I think my splitting way is good enough.
OK I will give response to your questions firstly.

Regards,
Ray

-Original Message-
From: Leif Lindholm [mailto:leif.lindh...@linaro.org] 
Sent: Tuesday, November 3, 2015 6:45 PM
To: Ni, Ruiyu <ruiyu...@intel.com>
Cc: Tian, Feng <feng.t...@intel.com>; Kinney, Michael D 
<michael.d.kin...@intel.com>; edk2-devel@lists.01.org; Fan, Jeff 
<jeff@intel.com>
Subject: Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug

Hi guys,

I never saw a response to this feedback I sent on the revised patch
set, but I see it has now been committed.
Could you let me know why my feedback was ignored?

Regards,

Leif

On 30 October 2015 at 18:02, Leif Lindholm <leif.lindh...@linaro.org> wrote:
> Hi Ray,
>
> Many thanks for this.
>
> On Fri, Oct 30, 2015 at 04:53:54AM +, Ni, Ruiyu wrote:
>> Ok I will revert the two patches I committed (big patch + small
>> patch). But I want to clarify one thing that not every big patch can
>> be easily understood by just splitting to small patches.
>
> Certainly, but it is always helpful to split as much as is possible.
> The shorter a patch is to review, the more likely reviewers are to
> spot mistakes - especially when reviewing code they are not already
> very familiar with.
>
> As an example, I have split your set up a little bit further by
> breaking 2/3 into two separate patches.
> https://git.linaro.org/people/leif.lindholm/edk2.git/shortlog/refs/heads/ray-pci
>
> I have also tried to remove modifications completely unrelated to this
> fix - resulting in a difference between the tree with your current
> patch set and my variant as included below. The changes in your set
> are all valid changes, but they are not related to the fix.
>
> The method behind all of this madness is to make the output of "git
> blame" as relevant as possible, as well as making automated "git
> bisect" sessions for tracking down which specific change caused an
> issue more useful.
>
> Best Regards,
>
> Leif
>
>
> Diff between Ray's v2 and my version of it:
>
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c 
> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> index 12647be..523c698 100644
> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
> @@ -281,13 +281,13 @@ DumpResourceMap (
>IN UINTN ResourceCount
>)
>  {
> -  EFI_STATUS   Status;
> -  LIST_ENTRY   *Link;
> -  PCI_IO_DEVICE*Device;
> -  UINTNIndex;
> -  CHAR16   *Str;
> -  PCI_RESOURCE_NODE**ChildResources;
> -  UINTNChildResourceCount;
> +  EFI_STATUS   Status;^M
> +  LIST_ENTRY   *Link;^M
> +  PCI_IO_DEVICE*Device;^M
> +  UINTNIndex;^M
> +  CHAR16   *Str;^M
> +  PCI_RESOURCE_NODE**ChildResources;^M
> +  UINTNChildResourceCount;^M
>
>DEBUG ((EFI_D_INFO, "PciBus: Resource Map for "));
>
> @@ -976,7 +976,7 @@ PciScanBus (
>UINT8 Device;
>UINT8 Func;
>UINT64Address;
> -  UINT8 SecondBus;
> +  UINTN SecondBus;^M
>UINT8 PaddedSubBus;
>UINT16Register;
>UINTN HpIndex;
> @@ -1211,7 +1211,7 @@ PciScanBus (
>
>Status = PciScanBus (
>  PciDevice,
> -SecondBus,
> +(UINT8) (SecondBus),^M
>  SubBusNumber,
>  PaddedBusRange
>  );
> @@ -1227,7 +1227,7 @@ PciScanBus (
>if ((Attributes == EfiPaddingPciRootBridge) &&
>(State & EFI_HPC_STATE_ENABLED) != 0&&
>(State & EFI_HPC_STATE_INITIALIZED) != 0) {
> -*PaddedBusRange = (UINT8) ((UINT8) (BusRange) + *PaddedBusRange);
> +*PaddedBusRange = (UINT8) ((UINT8) (BusRange) 
> +*PaddedBusRange);^M
>} else {
>  //
>  // Reserve the larger one between the actual occupied bus
>  // number and padded bus number
> diff --git a/MdeModulePkg/Bus/Pci/PciBusDx

Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug

2015-10-30 Thread Leif Lindholm 
hael D 
> <michael.d.kin...@intel.com>; edk2-devel@lists.01.org; Fan, Jeff 
> <jeff@intel.com>; Tian, Feng <feng.t...@intel.com>
> Subject: RE: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug
> 
> Leif,
> 
> Thanks for raising this issue.
> 
> I agree with you that the patch should be split to small ones and make it 
> more readable. I also agree we should give community more time to review 
> those big patch before getting committed in.
> 
> Ray, who is the module owner, will follow up your suggestions.
> 
> Thanks
> Feng
> 
> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Leif 
> Lindholm
> Sent: Thursday, October 29, 2015 19:55
> To: Tian, Feng
> Cc: Ni, Ruiyu; Kinney, Michael D; edk2-devel@lists.01.org; Fan, Jeff
> Subject: Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug
> 
> Hi Feng,
> 
> On Thu, Oct 29, 2015 at 12:23:04AM +, Tian, Feng wrote:
> > Ray has sent the patch for review, you may ignore it. 
> > (http://article.gmane.org/gmane.comp.bios.edk2.devel/3484)
> 
> Yes, I see it has now been committed - which solves the known bug.
> 
> But this still leaves us with a clearly not sufficiently reviewed/tested 
> invasive patch in core PCI code, and a jumbled commit history.
> 
> Since I have not received any response when asking previously, I will ask 
> again: Can we please revert SVN r18658 and introduce this changeset in a more 
> reviewable form, with proper review/testing?
> 
> Regards,
> 
> Leif
> 
> > -----Original Message-----
> > From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
> > Leif Lindholm
> > Sent: Wednesday, October 28, 2015 22:51
> > To: Tian, Feng
> > Cc: Ni, Ruiyu; edk2-devel@lists.01.org; Fan, Jeff
> > Subject: Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug
> > 
> > Feng,
> > 
> > Any update on the below?
> > The hard crash bug is still in SVN r18690.
> > 
> > Regards,
> > 
> > Leif
> > 
> > On Mon, Oct 26, 2015 at 03:35:15PM +, Leif Lindholm wrote:
> > > Hi Ray,
> > > 
> > > On Mon, Oct 26, 2015 at 03:17:22PM +, Ni, Ruiyu wrote:
> > > > Can you add a null pointer check to
> > > > PciIoDevice->ResourcePaddingDescriptors before calling
> > > > DumpPpbPaddingResource? Does it help?
> > > 
> > > It removes the delay and the crash - thanks!
> > > 
> > > But it does nothing for the commit history ;)
> > > 
> > > Regards,
> > > 
> > > Leif
> > > 
> > > > > 在 2015年10月26日，21:13，Leif Lindholm <leif.lindh...@linaro.org> 写道：
> > > > > 
> > > > > Hi Ruiyu, Feng,
> > > > > 
> > > > > I am currently tracking down an issue on (at least) one of my 
> > > > > platforms - that happens with this (now committed) patch, but 
> > > > > not without it.
> > > > > 
> > > > > Symptoms are a _long_ delay, followed by an unaligned access in 
> > > > > (I
> > > > > think) DumpPpbPaddingResource.
> > > > > 
> > > > > Anyway, there could be other things playing in here - I'm 
> > > > > testing a new card in a platform I haven't previously tested cards.
> > > > > 
> > > > > But this is one large patch, which could have been split up into 
> > > > > multiple ones to make the introduced changes more reviewable:
> > > > > - There are both functional changes and whitespace fixups.
> > > > > - There are (text-only) changes to existing messages.
> > > > > - There is refactoring of internal APIs.
> > > > > 
> > > > > It is certainly too invasive a change to be committed ~32 
> > > > > minutes after having first been posted to the list.
> > > > > 
> > > > > Any chance we can revert this one and introduce it in smaller 
> > > > > portions, making the individual change sets more reviewable?
> > > > > 
> > > > > Regards,
> > > > > 
> > > > > Leif
> > > > > 
> > > > >> On Fri, Oct 23, 2015 at 03:57:40PM +0800, Ruiyu Ni wrote:
> > > > >> For a hot plug bridge with device attached, PciBusDxe driver 
> > > > >> reserves the resources which equal to the total amount of 
> > > > >> padding resource returned from HotPlug->GetResourcePadding() 
> &g

Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug

2015-10-29 Thread Leif Lindholm 
Hi Feng,

On Thu, Oct 29, 2015 at 12:23:04AM +, Tian, Feng wrote:
> Ray has sent the patch for review, you may ignore
> it. (http://article.gmane.org/gmane.comp.bios.edk2.devel/3484)

Yes, I see it has now been committed - which solves the known bug.

But this still leaves us with a clearly not sufficiently
reviewed/tested invasive patch in core PCI code, and a jumbled commit
history.

Since I have not received any response when asking previously, I will
ask again: Can we please revert SVN r18658 and introduce this
changeset in a more reviewable form, with proper review/testing?

Regards,

Leif

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Leif 
> Lindholm
> Sent: Wednesday, October 28, 2015 22:51
> To: Tian, Feng
> Cc: Ni, Ruiyu; edk2-devel@lists.01.org; Fan, Jeff
> Subject: Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug
> 
> Feng,
> 
> Any update on the below?
> The hard crash bug is still in SVN r18690.
> 
> Regards,
> 
> Leif
> 
> On Mon, Oct 26, 2015 at 03:35:15PM +, Leif Lindholm wrote:
> > Hi Ray,
> > 
> > On Mon, Oct 26, 2015 at 03:17:22PM +, Ni, Ruiyu wrote:
> > > Can you add a null pointer check to
> > > PciIoDevice->ResourcePaddingDescriptors before calling
> > > DumpPpbPaddingResource? Does it help?
> > 
> > It removes the delay and the crash - thanks!
> > 
> > But it does nothing for the commit history ;)
> > 
> > Regards,
> > 
> > Leif
> > 
> > > > 在 2015年10月26日，21:13，Leif Lindholm <leif.lindh...@linaro.org> 写道：
> > > > 
> > > > Hi Ruiyu, Feng,
> > > > 
> > > > I am currently tracking down an issue on (at least) one of my 
> > > > platforms - that happens with this (now committed) patch, but not 
> > > > without it.
> > > > 
> > > > Symptoms are a _long_ delay, followed by an unaligned access in (I
> > > > think) DumpPpbPaddingResource.
> > > > 
> > > > Anyway, there could be other things playing in here - I'm testing 
> > > > a new card in a platform I haven't previously tested cards.
> > > > 
> > > > But this is one large patch, which could have been split up into 
> > > > multiple ones to make the introduced changes more reviewable:
> > > > - There are both functional changes and whitespace fixups.
> > > > - There are (text-only) changes to existing messages.
> > > > - There is refactoring of internal APIs.
> > > > 
> > > > It is certainly too invasive a change to be committed ~32 minutes 
> > > > after having first been posted to the list.
> > > > 
> > > > Any chance we can revert this one and introduce it in smaller 
> > > > portions, making the individual change sets more reviewable?
> > > > 
> > > > Regards,
> > > > 
> > > > Leif
> > > > 
> > > >> On Fri, Oct 23, 2015 at 03:57:40PM +0800, Ruiyu Ni wrote:
> > > >> For a hot plug bridge with device attached, PciBusDxe driver 
> > > >> reserves the resources which equal to the total amount of padding 
> > > >> resource returned from HotPlug->GetResourcePadding() and the 
> > > >> actual occupied resource by the attached device. The behavior is 
> > > >> incorrect.
> > > >> Correct behavior is to reserve the bigger one between the padding 
> > > >> resource and the actual occupied resource.
> > > >> 
> > > >> Contributed-under: TianoCore Contribution Agreement 1.0
> > > >> Signed-off-by: Ruiyu Ni <ruiyu...@intel.com>
> > > >> Cc: Jeff Fan <jeff@intel.com>
> > > >> Cc: Feng Tian <feng.t...@intel.com>
> > > >> ---
> > > >> .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.c   |  76 +-
> > > >> .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.h   |  13 ++
> > > >> MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c| 159 
> > > >> ++---
> > > >> .../Bus/Pci/PciBusDxe/PciResourceSupport.c |  58 +---
> > > >> 4 files changed, 204 insertions(+), 102 deletions(-)
> > > >> 
> > > >> diff --git 
> > > >> a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c 
> > > >> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > > >> index f7aea4f..030ef42 100644
> > > >> --- a/MdeModulePkg/Bus

Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug

2015-10-29 Thread Ni, Ruiyu 
Leif, Feng,
Ok I will revert the two patches I committed (big patch + small patch). But I 
want to clarify one thing that not every big patch can be easily understood by 
just splitting to small patches.

Thanks,
Ray

-Original Message-
From: Tian, Feng 
Sent: Friday, October 30, 2015 9:57 AM
To: Leif Lindholm <leif.lindh...@linaro.org>
Cc: Ni, Ruiyu <ruiyu...@intel.com>; Kinney, Michael D 
<michael.d.kin...@intel.com>; edk2-devel@lists.01.org; Fan, Jeff 
<jeff@intel.com>; Tian, Feng <feng.t...@intel.com>
Subject: RE: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug

Leif,

Thanks for raising this issue.

I agree with you that the patch should be split to small ones and make it more 
readable. I also agree we should give community more time to review those big 
patch before getting committed in.

Ray, who is the module owner, will follow up your suggestions.

Thanks
Feng

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Leif 
Lindholm
Sent: Thursday, October 29, 2015 19:55
To: Tian, Feng
Cc: Ni, Ruiyu; Kinney, Michael D; edk2-devel@lists.01.org; Fan, Jeff
Subject: Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug

Hi Feng,

On Thu, Oct 29, 2015 at 12:23:04AM +, Tian, Feng wrote:
> Ray has sent the patch for review, you may ignore it. 
> (http://article.gmane.org/gmane.comp.bios.edk2.devel/3484)

Yes, I see it has now been committed - which solves the known bug.

But this still leaves us with a clearly not sufficiently reviewed/tested 
invasive patch in core PCI code, and a jumbled commit history.

Since I have not received any response when asking previously, I will ask 
again: Can we please revert SVN r18658 and introduce this changeset in a more 
reviewable form, with proper review/testing?

Regards,

Leif

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
> Leif Lindholm
> Sent: Wednesday, October 28, 2015 22:51
> To: Tian, Feng
> Cc: Ni, Ruiyu; edk2-devel@lists.01.org; Fan, Jeff
> Subject: Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug
> 
> Feng,
> 
> Any update on the below?
> The hard crash bug is still in SVN r18690.
> 
> Regards,
> 
> Leif
> 
> On Mon, Oct 26, 2015 at 03:35:15PM +, Leif Lindholm wrote:
> > Hi Ray,
> > 
> > On Mon, Oct 26, 2015 at 03:17:22PM +, Ni, Ruiyu wrote:
> > > Can you add a null pointer check to
> > > PciIoDevice->ResourcePaddingDescriptors before calling
> > > DumpPpbPaddingResource? Does it help?
> > 
> > It removes the delay and the crash - thanks!
> > 
> > But it does nothing for the commit history ;)
> > 
> > Regards,
> > 
> > Leif
> > 
> > > > 在 2015年10月26日，21:13，Leif Lindholm <leif.lindh...@linaro.org> 写道：
> > > > 
> > > > Hi Ruiyu, Feng,
> > > > 
> > > > I am currently tracking down an issue on (at least) one of my 
> > > > platforms - that happens with this (now committed) patch, but 
> > > > not without it.
> > > > 
> > > > Symptoms are a _long_ delay, followed by an unaligned access in 
> > > > (I
> > > > think) DumpPpbPaddingResource.
> > > > 
> > > > Anyway, there could be other things playing in here - I'm 
> > > > testing a new card in a platform I haven't previously tested cards.
> > > > 
> > > > But this is one large patch, which could have been split up into 
> > > > multiple ones to make the introduced changes more reviewable:
> > > > - There are both functional changes and whitespace fixups.
> > > > - There are (text-only) changes to existing messages.
> > > > - There is refactoring of internal APIs.
> > > > 
> > > > It is certainly too invasive a change to be committed ~32 
> > > > minutes after having first been posted to the list.
> > > > 
> > > > Any chance we can revert this one and introduce it in smaller 
> > > > portions, making the individual change sets more reviewable?
> > > > 
> > > > Regards,
> > > > 
> > > > Leif
> > > > 
> > > >> On Fri, Oct 23, 2015 at 03:57:40PM +0800, Ruiyu Ni wrote:
> > > >> For a hot plug bridge with device attached, PciBusDxe driver 
> > > >> reserves the resources which equal to the total amount of 
> > > >> padding resource returned from HotPlug->GetResourcePadding() 
> > > >> and the actual occupied resource by the attached device. The behavior 
> > > >> is incorrect.
> > > >> Correct behavi

Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug

2015-10-29 Thread Tian, Feng 
Leif,

Thanks for raising this issue.

I agree with you that the patch should be split to small ones and make it more 
readable. I also agree we should give community more time to review those big 
patch before getting committed in.

Ray, who is the module owner, will follow up your suggestions.

Thanks
Feng

-Original Message-
From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Leif 
Lindholm
Sent: Thursday, October 29, 2015 19:55
To: Tian, Feng
Cc: Ni, Ruiyu; Kinney, Michael D; edk2-devel@lists.01.org; Fan, Jeff
Subject: Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug

Hi Feng,

On Thu, Oct 29, 2015 at 12:23:04AM +, Tian, Feng wrote:
> Ray has sent the patch for review, you may ignore it. 
> (http://article.gmane.org/gmane.comp.bios.edk2.devel/3484)

Yes, I see it has now been committed - which solves the known bug.

But this still leaves us with a clearly not sufficiently reviewed/tested 
invasive patch in core PCI code, and a jumbled commit history.

Since I have not received any response when asking previously, I will ask 
again: Can we please revert SVN r18658 and introduce this changeset in a more 
reviewable form, with proper review/testing?

Regards,

Leif

> -Original Message-
> From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of 
> Leif Lindholm
> Sent: Wednesday, October 28, 2015 22:51
> To: Tian, Feng
> Cc: Ni, Ruiyu; edk2-devel@lists.01.org; Fan, Jeff
> Subject: Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug
> 
> Feng,
> 
> Any update on the below?
> The hard crash bug is still in SVN r18690.
> 
> Regards,
> 
> Leif
> 
> On Mon, Oct 26, 2015 at 03:35:15PM +, Leif Lindholm wrote:
> > Hi Ray,
> > 
> > On Mon, Oct 26, 2015 at 03:17:22PM +, Ni, Ruiyu wrote:
> > > Can you add a null pointer check to
> > > PciIoDevice->ResourcePaddingDescriptors before calling
> > > DumpPpbPaddingResource? Does it help?
> > 
> > It removes the delay and the crash - thanks!
> > 
> > But it does nothing for the commit history ;)
> > 
> > Regards,
> > 
> > Leif
> > 
> > > > 在 2015年10月26日，21:13，Leif Lindholm <leif.lindh...@linaro.org> 写道：
> > > > 
> > > > Hi Ruiyu, Feng,
> > > > 
> > > > I am currently tracking down an issue on (at least) one of my 
> > > > platforms - that happens with this (now committed) patch, but 
> > > > not without it.
> > > > 
> > > > Symptoms are a _long_ delay, followed by an unaligned access in 
> > > > (I
> > > > think) DumpPpbPaddingResource.
> > > > 
> > > > Anyway, there could be other things playing in here - I'm 
> > > > testing a new card in a platform I haven't previously tested cards.
> > > > 
> > > > But this is one large patch, which could have been split up into 
> > > > multiple ones to make the introduced changes more reviewable:
> > > > - There are both functional changes and whitespace fixups.
> > > > - There are (text-only) changes to existing messages.
> > > > - There is refactoring of internal APIs.
> > > > 
> > > > It is certainly too invasive a change to be committed ~32 
> > > > minutes after having first been posted to the list.
> > > > 
> > > > Any chance we can revert this one and introduce it in smaller 
> > > > portions, making the individual change sets more reviewable?
> > > > 
> > > > Regards,
> > > > 
> > > > Leif
> > > > 
> > > >> On Fri, Oct 23, 2015 at 03:57:40PM +0800, Ruiyu Ni wrote:
> > > >> For a hot plug bridge with device attached, PciBusDxe driver 
> > > >> reserves the resources which equal to the total amount of 
> > > >> padding resource returned from HotPlug->GetResourcePadding() 
> > > >> and the actual occupied resource by the attached device. The behavior 
> > > >> is incorrect.
> > > >> Correct behavior is to reserve the bigger one between the 
> > > >> padding resource and the actual occupied resource.
> > > >> 
> > > >> Contributed-under: TianoCore Contribution Agreement 1.0
> > > >> Signed-off-by: Ruiyu Ni <ruiyu...@intel.com>
> > > >> Cc: Jeff Fan <jeff@intel.com>
> > > >> Cc: Feng Tian <feng.t...@intel.com>
> > > >> ---
> > > >> .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.c   |  76 +-
> > > >> .../Bus/Pci/PciBusDxe/PciEnumeratorSuppo

Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug

2015-10-28 Thread Leif Lindholm 
Feng,

Any update on the below?
The hard crash bug is still in SVN r18690.

Regards,

Leif

On Mon, Oct 26, 2015 at 03:35:15PM +, Leif Lindholm wrote:
> Hi Ray,
> 
> On Mon, Oct 26, 2015 at 03:17:22PM +, Ni, Ruiyu wrote:
> > Can you add a null pointer check to
> > PciIoDevice->ResourcePaddingDescriptors before calling
> > DumpPpbPaddingResource? Does it help?
> 
> It removes the delay and the crash - thanks!
> 
> But it does nothing for the commit history ;)
> 
> Regards,
> 
> Leif
> 
> > > 在 2015年10月26日，21:13，Leif Lindholm  写道：
> > > 
> > > Hi Ruiyu, Feng,
> > > 
> > > I am currently tracking down an issue on (at least) one of my
> > > platforms - that happens with this (now committed) patch, but not
> > > without it.
> > > 
> > > Symptoms are a _long_ delay, followed by an unaligned access in (I
> > > think) DumpPpbPaddingResource.
> > > 
> > > Anyway, there could be other things playing in here - I'm testing a
> > > new card in a platform I haven't previously tested cards.
> > > 
> > > But this is one large patch, which could have been split up into
> > > multiple ones to make the introduced changes more reviewable:
> > > - There are both functional changes and whitespace fixups.
> > > - There are (text-only) changes to existing messages.
> > > - There is refactoring of internal APIs.
> > > 
> > > It is certainly too invasive a change to be committed ~32 minutes
> > > after having first been posted to the list.
> > > 
> > > Any chance we can revert this one and introduce it in smaller
> > > portions, making the individual change sets more reviewable?
> > > 
> > > Regards,
> > > 
> > > Leif
> > > 
> > >> On Fri, Oct 23, 2015 at 03:57:40PM +0800, Ruiyu Ni wrote:
> > >> For a hot plug bridge with device attached, PciBusDxe driver reserves
> > >> the resources which equal to the total amount of padding resource
> > >> returned from HotPlug->GetResourcePadding() and the actual occupied
> > >> resource by the attached device. The behavior is incorrect.
> > >> Correct behavior is to reserve the bigger one between the padding
> > >> resource and the actual occupied resource.
> > >> 
> > >> Contributed-under: TianoCore Contribution Agreement 1.0
> > >> Signed-off-by: Ruiyu Ni 
> > >> Cc: Jeff Fan 
> > >> Cc: Feng Tian 
> > >> ---
> > >> .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.c   |  76 +-
> > >> .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.h   |  13 ++
> > >> MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c| 159 
> > >> ++---
> > >> .../Bus/Pci/PciBusDxe/PciResourceSupport.c |  58 +---
> > >> 4 files changed, 204 insertions(+), 102 deletions(-)
> > >> 
> > >> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c 
> > >> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > >> index f7aea4f..030ef42 100644
> > >> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > >> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> > >> @@ -325,6 +325,77 @@ PciSearchDevice (
> > >> }
> > >> 
> > >> /**
> > >> +  Dump the PPB padding resource information.
> > >> +
> > >> +  @param PciIoDevice PCI IO instance.
> > >> +  @param ResourceTypeThe desired resource type to dump.
> > >> + PciBarTypeUnknown means to dump all types of 
> > >> resources.
> > >> +**/
> > >> +VOID
> > >> +DumpPpbPaddingResource (
> > >> +  IN PCI_IO_DEVICE*PciIoDevice,
> > >> +  IN PCI_BAR_TYPE ResourceType
> > >> +  )
> > >> +{
> > >> +  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptor;
> > >> +  PCI_BAR_TYPE  Type;
> > >> +
> > >> +  if (ResourceType == PciBarTypeIo16 || ResourceType == PciBarTypeIo32) 
> > >> {
> > >> +ResourceType = PciBarTypeIo;
> > >> +  }
> > >> +
> > >> +  for (Descriptor = PciIoDevice->ResourcePaddingDescriptors; 
> > >> Descriptor->Desc != ACPI_END_TAG_DESCRIPTOR; Descriptor++) {
> > >> +
> > >> +Type = PciBarTypeUnknown;
> > >> +if (Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR && 
> > >> Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_IO) {
> > >> +  Type = PciBarTypeIo;
> > >> +} else if (Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR && 
> > >> Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) {
> > >> +
> > >> +  if (Descriptor->AddrSpaceGranularity == 32) {
> > >> +//
> > >> +// prefechable
> > >> +//
> > >> +if (Descriptor->SpecificFlag == 
> > >> EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) {
> > >> +  Type = PciBarTypePMem32;
> > >> +}
> > >> +
> > >> +//
> > >> +// Non-prefechable
> > >> +//
> > >> +if (Descriptor->SpecificFlag == 0) {
> > >> +  Type = PciBarTypeMem32;
> > >> +}
> > >> +  }
> > >> +
> > >> +  if (Descriptor->AddrSpaceGranularity == 64) {
> > >> +//
> > >> +//

Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug

2015-10-28 Thread Kinney, Michael D 
Leif,

Please test the patch I just sent to see if it resolves your issue.

Thanks,

Mike

>-Original Message-
>From: edk2-devel [mailto:edk2-devel-boun...@lists.01.org] On Behalf Of Leif
>Lindholm
>Sent: Wednesday, October 28, 2015 7:51 AM
>To: Tian, Feng
>Cc: Ni, Ruiyu; edk2-devel@lists.01.org; Fan, Jeff
>Subject: Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug
>
>Feng,
>
>Any update on the below?
>The hard crash bug is still in SVN r18690.
>
>Regards,
>
>Leif
>
>On Mon, Oct 26, 2015 at 03:35:15PM +, Leif Lindholm wrote:
>> Hi Ray,
>>
>> On Mon, Oct 26, 2015 at 03:17:22PM +, Ni, Ruiyu wrote:
>> > Can you add a null pointer check to
>> > PciIoDevice->ResourcePaddingDescriptors before calling
>> > DumpPpbPaddingResource? Does it help?
>>
>> It removes the delay and the crash - thanks!
>>
>> But it does nothing for the commit history ;)
>>
>> Regards,
>>
>> Leif
>>
>> > > 在 2015年10月26日，21:13，Leif Lindholm <leif.lindh...@linaro.org>
>写道：
>> > >
>> > > Hi Ruiyu, Feng,
>> > >
>> > > I am currently tracking down an issue on (at least) one of my
>> > > platforms - that happens with this (now committed) patch, but not
>> > > without it.
>> > >
>> > > Symptoms are a _long_ delay, followed by an unaligned access in (I
>> > > think) DumpPpbPaddingResource.
>> > >
>> > > Anyway, there could be other things playing in here - I'm testing a
>> > > new card in a platform I haven't previously tested cards.
>> > >
>> > > But this is one large patch, which could have been split up into
>> > > multiple ones to make the introduced changes more reviewable:
>> > > - There are both functional changes and whitespace fixups.
>> > > - There are (text-only) changes to existing messages.
>> > > - There is refactoring of internal APIs.
>> > >
>> > > It is certainly too invasive a change to be committed ~32 minutes
>> > > after having first been posted to the list.
>> > >
>> > > Any chance we can revert this one and introduce it in smaller
>> > > portions, making the individual change sets more reviewable?
>> > >
>> > > Regards,
>> > >
>> > > Leif
>> > >
>> > >> On Fri, Oct 23, 2015 at 03:57:40PM +0800, Ruiyu Ni wrote:
>> > >> For a hot plug bridge with device attached, PciBusDxe driver reserves
>> > >> the resources which equal to the total amount of padding resource
>> > >> returned from HotPlug->GetResourcePadding() and the actual occupied
>> > >> resource by the attached device. The behavior is incorrect.
>> > >> Correct behavior is to reserve the bigger one between the padding
>> > >> resource and the actual occupied resource.
>> > >>
>> > >> Contributed-under: TianoCore Contribution Agreement 1.0
>> > >> Signed-off-by: Ruiyu Ni <ruiyu...@intel.com>
>> > >> Cc: Jeff Fan <jeff@intel.com>
>> > >> Cc: Feng Tian <feng.t...@intel.com>
>> > >> ---
>> > >> .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.c   |  76 +-
>> > >> .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.h   |  13 ++
>> > >> MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c| 159 ++-
>--
>> > >> .../Bus/Pci/PciBusDxe/PciResourceSupport.c |  58 +---
>> > >> 4 files changed, 204 insertions(+), 102 deletions(-)
>> > >>
>> > >> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
>b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
>> > >> index f7aea4f..030ef42 100644
>> > >> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
>> > >> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
>> > >> @@ -325,6 +325,77 @@ PciSearchDevice (
>> > >> }
>> > >>
>> > >> /**
>> > >> +  Dump the PPB padding resource information.
>> > >> +
>> > >> +  @param PciIoDevice PCI IO instance.
>> > >> +  @param ResourceTypeThe desired resource type to dump.
>> > >> + PciBarTypeUnknown means to dump all types of
>resources.
>> > >> +**/
>> > >> +VOID
>> > >> +DumpPpbPaddingResou

Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug

2015-10-26 Thread Ni, Ruiyu 
Can you add a null pointer check to  PciIoDevice->ResourcePaddingDescriptors 
before calling DumpPpbPaddingResource? Does it help?

Thanks,
Ray

> 在 2015年10月26日，21:13，Leif Lindholm  写道：
> 
> Hi Ruiyu, Feng,
> 
> I am currently tracking down an issue on (at least) one of my
> platforms - that happens with this (now committed) patch, but not
> without it.
> 
> Symptoms are a _long_ delay, followed by an unaligned access in (I
> think) DumpPpbPaddingResource.
> 
> Anyway, there could be other things playing in here - I'm testing a
> new card in a platform I haven't previously tested cards.
> 
> But this is one large patch, which could have been split up into
> multiple ones to make the introduced changes more reviewable:
> - There are both functional changes and whitespace fixups.
> - There are (text-only) changes to existing messages.
> - There is refactoring of internal APIs.
> 
> It is certainly too invasive a change to be committed ~32 minutes
> after having first been posted to the list.
> 
> Any chance we can revert this one and introduce it in smaller
> portions, making the individual change sets more reviewable?
> 
> Regards,
> 
> Leif
> 
>> On Fri, Oct 23, 2015 at 03:57:40PM +0800, Ruiyu Ni wrote:
>> For a hot plug bridge with device attached, PciBusDxe driver reserves
>> the resources which equal to the total amount of padding resource
>> returned from HotPlug->GetResourcePadding() and the actual occupied
>> resource by the attached device. The behavior is incorrect.
>> Correct behavior is to reserve the bigger one between the padding
>> resource and the actual occupied resource.
>> 
>> Contributed-under: TianoCore Contribution Agreement 1.0
>> Signed-off-by: Ruiyu Ni 
>> Cc: Jeff Fan 
>> Cc: Feng Tian 
>> ---
>> .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.c   |  76 +-
>> .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.h   |  13 ++
>> MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c| 159 
>> ++---
>> .../Bus/Pci/PciBusDxe/PciResourceSupport.c |  58 +---
>> 4 files changed, 204 insertions(+), 102 deletions(-)
>> 
>> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c 
>> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
>> index f7aea4f..030ef42 100644
>> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
>> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
>> @@ -325,6 +325,77 @@ PciSearchDevice (
>> }
>> 
>> /**
>> +  Dump the PPB padding resource information.
>> +
>> +  @param PciIoDevice PCI IO instance.
>> +  @param ResourceTypeThe desired resource type to dump.
>> + PciBarTypeUnknown means to dump all types of 
>> resources.
>> +**/
>> +VOID
>> +DumpPpbPaddingResource (
>> +  IN PCI_IO_DEVICE*PciIoDevice,
>> +  IN PCI_BAR_TYPE ResourceType
>> +  )
>> +{
>> +  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptor;
>> +  PCI_BAR_TYPE  Type;
>> +
>> +  if (ResourceType == PciBarTypeIo16 || ResourceType == PciBarTypeIo32) {
>> +ResourceType = PciBarTypeIo;
>> +  }
>> +
>> +  for (Descriptor = PciIoDevice->ResourcePaddingDescriptors; 
>> Descriptor->Desc != ACPI_END_TAG_DESCRIPTOR; Descriptor++) {
>> +
>> +Type = PciBarTypeUnknown;
>> +if (Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR && 
>> Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_IO) {
>> +  Type = PciBarTypeIo;
>> +} else if (Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR && 
>> Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) {
>> +
>> +  if (Descriptor->AddrSpaceGranularity == 32) {
>> +//
>> +// prefechable
>> +//
>> +if (Descriptor->SpecificFlag == 
>> EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) {
>> +  Type = PciBarTypePMem32;
>> +}
>> +
>> +//
>> +// Non-prefechable
>> +//
>> +if (Descriptor->SpecificFlag == 0) {
>> +  Type = PciBarTypeMem32;
>> +}
>> +  }
>> +
>> +  if (Descriptor->AddrSpaceGranularity == 64) {
>> +//
>> +// prefechable
>> +//
>> +if (Descriptor->SpecificFlag == 
>> EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) {
>> +  Type = PciBarTypePMem64;
>> +}
>> +
>> +//
>> +// Non-prefechable
>> +//
>> +if (Descriptor->SpecificFlag == 0) {
>> +  Type = PciBarTypeMem64;
>> +}
>> +  }
>> +}
>> +
>> +if ((Type != PciBarTypeUnknown) && ((ResourceType == PciBarTypeUnknown) 
>> || (ResourceType == Type))) {
>> +  DEBUG ((
>> +EFI_D_INFO,
>> +"   Padding: Type = %s; Alignment = 0x%lx;\tLength = 0x%lx\n",
>> +mBarTypeStr[Type], Descriptor->AddrRangeMax, Descriptor->AddrLen
>> +));
>> +}
>> +  }
>> +
>> +}
>> +
>> +/**
>>   Dump the PCI BAR information.
>> 
>>   @param

Re: [edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug

2015-10-26 Thread Leif Lindholm 
Hi Ray,

On Mon, Oct 26, 2015 at 03:17:22PM +, Ni, Ruiyu wrote:
> Can you add a null pointer check to
> PciIoDevice->ResourcePaddingDescriptors before calling
> DumpPpbPaddingResource? Does it help?

It removes the delay and the crash - thanks!

But it does nothing for the commit history ;)

Regards,

Leif

> > 在 2015年10月26日，21:13，Leif Lindholm  写道：
> > 
> > Hi Ruiyu, Feng,
> > 
> > I am currently tracking down an issue on (at least) one of my
> > platforms - that happens with this (now committed) patch, but not
> > without it.
> > 
> > Symptoms are a _long_ delay, followed by an unaligned access in (I
> > think) DumpPpbPaddingResource.
> > 
> > Anyway, there could be other things playing in here - I'm testing a
> > new card in a platform I haven't previously tested cards.
> > 
> > But this is one large patch, which could have been split up into
> > multiple ones to make the introduced changes more reviewable:
> > - There are both functional changes and whitespace fixups.
> > - There are (text-only) changes to existing messages.
> > - There is refactoring of internal APIs.
> > 
> > It is certainly too invasive a change to be committed ~32 minutes
> > after having first been posted to the list.
> > 
> > Any chance we can revert this one and introduce it in smaller
> > portions, making the individual change sets more reviewable?
> > 
> > Regards,
> > 
> > Leif
> > 
> >> On Fri, Oct 23, 2015 at 03:57:40PM +0800, Ruiyu Ni wrote:
> >> For a hot plug bridge with device attached, PciBusDxe driver reserves
> >> the resources which equal to the total amount of padding resource
> >> returned from HotPlug->GetResourcePadding() and the actual occupied
> >> resource by the attached device. The behavior is incorrect.
> >> Correct behavior is to reserve the bigger one between the padding
> >> resource and the actual occupied resource.
> >> 
> >> Contributed-under: TianoCore Contribution Agreement 1.0
> >> Signed-off-by: Ruiyu Ni 
> >> Cc: Jeff Fan 
> >> Cc: Feng Tian 
> >> ---
> >> .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.c   |  76 +-
> >> .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.h   |  13 ++
> >> MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c| 159 
> >> ++---
> >> .../Bus/Pci/PciBusDxe/PciResourceSupport.c |  58 +---
> >> 4 files changed, 204 insertions(+), 102 deletions(-)
> >> 
> >> diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c 
> >> b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> >> index f7aea4f..030ef42 100644
> >> --- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> >> +++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
> >> @@ -325,6 +325,77 @@ PciSearchDevice (
> >> }
> >> 
> >> /**
> >> +  Dump the PPB padding resource information.
> >> +
> >> +  @param PciIoDevice PCI IO instance.
> >> +  @param ResourceTypeThe desired resource type to dump.
> >> + PciBarTypeUnknown means to dump all types of 
> >> resources.
> >> +**/
> >> +VOID
> >> +DumpPpbPaddingResource (
> >> +  IN PCI_IO_DEVICE*PciIoDevice,
> >> +  IN PCI_BAR_TYPE ResourceType
> >> +  )
> >> +{
> >> +  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptor;
> >> +  PCI_BAR_TYPE  Type;
> >> +
> >> +  if (ResourceType == PciBarTypeIo16 || ResourceType == PciBarTypeIo32) {
> >> +ResourceType = PciBarTypeIo;
> >> +  }
> >> +
> >> +  for (Descriptor = PciIoDevice->ResourcePaddingDescriptors; 
> >> Descriptor->Desc != ACPI_END_TAG_DESCRIPTOR; Descriptor++) {
> >> +
> >> +Type = PciBarTypeUnknown;
> >> +if (Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR && 
> >> Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_IO) {
> >> +  Type = PciBarTypeIo;
> >> +} else if (Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR && 
> >> Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) {
> >> +
> >> +  if (Descriptor->AddrSpaceGranularity == 32) {
> >> +//
> >> +// prefechable
> >> +//
> >> +if (Descriptor->SpecificFlag == 
> >> EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) {
> >> +  Type = PciBarTypePMem32;
> >> +}
> >> +
> >> +//
> >> +// Non-prefechable
> >> +//
> >> +if (Descriptor->SpecificFlag == 0) {
> >> +  Type = PciBarTypeMem32;
> >> +}
> >> +  }
> >> +
> >> +  if (Descriptor->AddrSpaceGranularity == 64) {
> >> +//
> >> +// prefechable
> >> +//
> >> +if (Descriptor->SpecificFlag == 
> >> EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) {
> >> +  Type = PciBarTypePMem64;
> >> +}
> >> +
> >> +//
> >> +// Non-prefechable
> >> +//
> >> +if (Descriptor->SpecificFlag == 0) {
> >> +  Type = PciBarTypeMem64;
> >> +}
> >> +  }
> >> +}
> >> +
>

[edk2] [Patch] MdeModulePkg: Fix a PciBusDxe hot plug bug

2015-10-23 Thread Ruiyu Ni 
For a hot plug bridge with device attached, PciBusDxe driver reserves
the resources which equal to the total amount of padding resource
returned from HotPlug->GetResourcePadding() and the actual occupied
resource by the attached device. The behavior is incorrect.
Correct behavior is to reserve the bigger one between the padding
resource and the actual occupied resource.

Contributed-under: TianoCore Contribution Agreement 1.0
Signed-off-by: Ruiyu Ni 
Cc: Jeff Fan 
Cc: Feng Tian 
---
 .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.c   |  76 +-
 .../Bus/Pci/PciBusDxe/PciEnumeratorSupport.h   |  13 ++
 MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c| 159 ++---
 .../Bus/Pci/PciBusDxe/PciResourceSupport.c |  58 +---
 4 files changed, 204 insertions(+), 102 deletions(-)

diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c 
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
index f7aea4f..030ef42 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.c
@@ -325,6 +325,77 @@ PciSearchDevice (
 }
 
 /**
+  Dump the PPB padding resource information.
+
+  @param PciIoDevice PCI IO instance.
+  @param ResourceTypeThe desired resource type to dump.
+ PciBarTypeUnknown means to dump all types of 
resources.
+**/
+VOID
+DumpPpbPaddingResource (
+  IN PCI_IO_DEVICE*PciIoDevice,
+  IN PCI_BAR_TYPE ResourceType
+  )
+{
+  EFI_ACPI_ADDRESS_SPACE_DESCRIPTOR *Descriptor;
+  PCI_BAR_TYPE  Type;
+
+  if (ResourceType == PciBarTypeIo16 || ResourceType == PciBarTypeIo32) {
+ResourceType = PciBarTypeIo;
+  }
+
+  for (Descriptor = PciIoDevice->ResourcePaddingDescriptors; Descriptor->Desc 
!= ACPI_END_TAG_DESCRIPTOR; Descriptor++) {
+
+Type = PciBarTypeUnknown;
+if (Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR && 
Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_IO) {
+  Type = PciBarTypeIo;
+} else if (Descriptor->Desc == ACPI_ADDRESS_SPACE_DESCRIPTOR && 
Descriptor->ResType == ACPI_ADDRESS_SPACE_TYPE_MEM) {
+
+  if (Descriptor->AddrSpaceGranularity == 32) {
+//
+// prefechable
+//
+if (Descriptor->SpecificFlag == 
EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) {
+  Type = PciBarTypePMem32;
+}
+
+//
+// Non-prefechable
+//
+if (Descriptor->SpecificFlag == 0) {
+  Type = PciBarTypeMem32;
+}
+  }
+
+  if (Descriptor->AddrSpaceGranularity == 64) {
+//
+// prefechable
+//
+if (Descriptor->SpecificFlag == 
EFI_ACPI_MEMORY_RESOURCE_SPECIFIC_FLAG_CACHEABLE_PREFETCHABLE) {
+  Type = PciBarTypePMem64;
+}
+
+//
+// Non-prefechable
+//
+if (Descriptor->SpecificFlag == 0) {
+  Type = PciBarTypeMem64;
+}
+  }
+}
+
+if ((Type != PciBarTypeUnknown) && ((ResourceType == PciBarTypeUnknown) || 
(ResourceType == Type))) {
+  DEBUG ((
+EFI_D_INFO,
+"   Padding: Type = %s; Alignment = 0x%lx;\tLength = 0x%lx\n",
+mBarTypeStr[Type], Descriptor->AddrRangeMax, Descriptor->AddrLen
+));
+}
+  }
+
+}
+
+/**
   Dump the PCI BAR information.
 
   @param PciIoDevice PCI IO instance.
@@ -586,7 +657,10 @@ GatherPpbInfo (
 
   GetResourcePaddingPpb (PciIoDevice);
 
-  DEBUG_CODE (DumpPciBars (PciIoDevice););
+  DEBUG_CODE (
+DumpPpbPaddingResource (PciIoDevice, PciBarTypeUnknown);
+DumpPciBars (PciIoDevice);
+  );
 
   return PciIoDevice;
 }
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.h 
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.h
index a4489b8..4d7b3b7 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.h
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciEnumeratorSupport.h
@@ -460,4 +460,17 @@ ResetAllPpbBusNumber (
   IN UINT8  StartBusNumber
   );
 
+/**
+  Dump the PPB padding resource information.
+
+  @param PciIoDevice PCI IO instance.
+  @param ResourceTypeThe desired resource type to dump.
+ PciBarTypeUnknown means to dump all types of 
resources.
+**/
+VOID
+DumpPpbPaddingResource (
+  IN PCI_IO_DEVICE*PciIoDevice,
+  IN PCI_BAR_TYPE ResourceType
+  );
+
 #endif
diff --git a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c 
b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
index 3e275e3..f4b6ebf 100644
--- a/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
+++ b/MdeModulePkg/Bus/Pci/PciBusDxe/PciLib.c
@@ -188,19 +188,21 @@ DumpBridgeResource (
   BridgeResource->PciDev->PciBar[BridgeResource->Bar].BaseAddress,
   BridgeResource->Length, BridgeResource->Alignment
   ));
-for ( Link = BridgeResource->ChildList.ForwardLink
-; Link !=