Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1310292 > unrolled thread

RE: [PATCH V3 1/2] NTB: Add AMD PCI-Express NTB driver

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  RE: [PATCH V3 1/2] NTB: Add AMD PCI-Express NTB driver "Allen Hubbe" <Allen.Hubbe@emc.com> - 2016-01-15 18:20 +0100
    RE: [PATCH V3 1/2] NTB: Add AMD PCI-Express NTB driver "Yu, Xiangliang" <Xiangliang.Yu@amd.com> - 2016-01-18 07:50 +0100

#1310292 — RE: [PATCH V3 1/2] NTB: Add AMD PCI-Express NTB driver

> From: Yu, Xiangliang <Xiangliang.Yu@amd.com>
> 
> Sorry, let me clarify it: one 16-bit doorbell is for primary side and
> the other is be used
> By secondary side. The #3 should be is that two 16-bit doorbell
> registers.

Thanks!  I think I understand now.

> In here, i list all features AMD NTB support, not patch. Please see my
> word again.

This is the commit message for this patch.  It would be confusing to have features mentioned in the commit message not implemented, and no explanation why not.

> > What is SMU?
> 
> System management unit, please reference AMD manual.

Thanks, but it is your responsibility to support this driver, and respond to questions like this.  I didn't write it, I can't test it, and I don't have a hardware manual.

If this is an ack for the system management unit, is it acknowledging a watchdog on the SMU?  Does this ack have to do with the flush or power management functions?  If it is a watchdog, what implications are there for unloading your driver - does the watchdog just time out, and then what, reset the device?  I'm speculating (guessing) again, but that's really the best I can do without your authoritative answers, or a hardware manual.

Where can the hardware manual be found?  Has it been published, or is it AMD confidential?

http://search.amd.com/en-us/Pages/results-all.aspx#k=zeppelin
http://search.amd.com/en-us/Pages/results-all.aspx#k=ntb

"Nothing here matches your search"

> > I would prefer to see #define, but this works... There are good
> arguments to
> > be made for compiler constants vs the preprocessor.  My preference
> just
> > comes from what I've observed about how other drivers are written, and
> > #define for this stuff seems to be the convention.
> 
> I have lots of SATA/Block layer experience, this following AHCI coding
> style.
> Compiler will check enum variable, not for macro.
> Actually, there are lots of similar definition in kernel, such as block
> layer code.

Thanks.  I'm ok with this.

> > It's unusual, sometimes dangerous, to use the static keyword in a .h
> file
> 
> Actually, I put it in .c file in first version, I think it make code
> clear if moving it to
> .h file. Yes, I can remove all these definitions by rearranging .c file.
> 
> And please speak out all your concern in this time, you know, I had
> spent lots of time
>  On these patches.

You're right.  I didn't notice this change from v1 to v2.  It would have been a more efficient code review had I made these comments in v2.

My comments this time around, as I said, were minor.  Each version of this patch series has been an improvement.  Maybe this version is even good enough, but there are /always/ comments to be made about code.  It's not unusual to see patches on the kernel mailing list with double digit reroll counts (I saw one at v12 today).  Sure, it can be frustrating.   Please don't take offense.  Thank you for your efforts.

The bad news:  I have two more comments.

Please run scritps/checkpatch.pl --strict with your patch.  There are some formatting issues.

Please add your name to the MAINTANERS file.  Add a section, NTB AMD DRIVER.  The checkpatch script also mentions this, but it's especially important.  You are the only one who can test this driver, with the simulator.

The good news:

I'm inclined to say yes to this patch.  It looks ok, and I'll take your word that it has been tested.  Ultimately, the patch has to be accepted by Jon and Linus.  By adding my comments, I'm just trying to help.

Do you think you can email v4 by tomorrow?  I'll be sure to respond quickly if you do, so it has the best chance to meet the deadline for the merge window.  It's getting close to the deadline, and I'll take some of the blame for that.

Allen

[toc] | [next] | [standalone]

#1311338

> > In here, i list all features AMD NTB support, not patch. Please see my
> > word again.
> 
> This is the commit message for this patch.  It would be confusing to have
> features mentioned in the commit message not implemented, and no
> explanation why not.

Ok, I'll change it.

> > > What is SMU?
> >
> > System management unit, please reference AMD manual.
> 
> Thanks, but it is your responsibility to support this driver, and respond to
> questions like this.  I didn't write it, I can't test it, and I don't have a hardware
> manual.
> 
> If this is an ack for the system management unit, is it acknowledging a
> watchdog on the SMU?  Does this ack have to do with the flush or power
> management functions?  If it is a watchdog, what implications are there for
> unloading your driver - does the watchdog just time out, and then what,
> reset the device?  I'm speculating (guessing) again, but that's really the best I
> can do without your authoritative answers, or a hardware manual.
> 
> Where can the hardware manual be found?  Has it been published, or is it
> AMD confidential?
> 
> http://search.amd.com/en-us/Pages/results-all.aspx#k=zeppelin
> http://search.amd.com/en-us/Pages/results-all.aspx#k=ntb
> 
> "Nothing here matches your search"

SMU is a big system, which including hardware and firmware. I think it is
Transport to NTB, just use its interface it expose and following NTB hardware
Specification. Right now, the specification hasn't been public released. I think
You need to request NDA( Non Disclosure Agreeement) license to review it.

> > > I would prefer to see #define, but this works... There are good
> > arguments to
> > > be made for compiler constants vs the preprocessor.  My preference
> > just
> > > comes from what I've observed about how other drivers are written,
> > > and #define for this stuff seems to be the convention.
> >
> > I have lots of SATA/Block layer experience, this following AHCI coding
> > style.
> > Compiler will check enum variable, not for macro.
> > Actually, there are lots of similar definition in kernel, such as
> > block layer code.
> 
> Thanks.  I'm ok with this.
> 
> > > It's unusual, sometimes dangerous, to use the static keyword in a .h
> > file
> >
> > Actually, I put it in .c file in first version, I think it make code
> > clear if moving it to .h file. Yes, I can remove all these definitions
> > by rearranging .c file.
> >
> > And please speak out all your concern in this time, you know, I had
> > spent lots of time  On these patches.
> 
> You're right.  I didn't notice this change from v1 to v2.  It would have been a
> more efficient code review had I made these comments in v2.
> 
> My comments this time around, as I said, were minor.  Each version of this
> patch series has been an improvement.  Maybe this version is even good
> enough, but there are /always/ comments to be made about code.  It's not
> unusual to see patches on the kernel mailing list with double digit reroll
> counts (I saw one at v12 today).  Sure, it can be frustrating.   Please don't take
> offense.  Thank you for your efforts.
> 
> The bad news:  I have two more comments.
> 
> Please run scritps/checkpatch.pl --strict with your patch.  There are some
> formatting issues.
> Please add your name to the MAINTANERS file.  Add a section, NTB AMD
> DRIVER.  The checkpatch script also mentions this, but it's especially
> important.  You are the only one who can test this driver, with the simulator.

Ok, I'll do it. 

> The good news:
> 
> I'm inclined to say yes to this patch.  It looks ok, and I'll take your word that it
> has been tested.  Ultimately, the patch has to be accepted by Jon and Linus.
> By adding my comments, I'm just trying to help.
> 
> Do you think you can email v4 by tomorrow?  I'll be sure to respond quickly if
> you do, so it has the best chance to meet the deadline for the merge window.
> It's getting close to the deadline, and I'll take some of the blame for that.

Because I was busying other things at weekend, I'll change it right now.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web