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

From: Yu, Xiangliang
Date: Mon Jan 18 2016 - 01:47:47 EST

> > 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?
> "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/ --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.