This patch does not change what any of these Makefiles add to LDFLAGS. What is being changed by this patch to have these Makefile add their options to any existing contents of $LDFLAGS by using "LDFLAGS += ..." rather than discarding any existing contents of $LDFLAGS by using "LDFLAGS = ..." . I have no idea why any of these Makefiles want to add anything to LDFLAGS or if there is a better way to accomplish their goal, all I want to change is to add the "+'" in between "LDFLAGS" and "=".

rebase to include recent changes to the tpm driver

This patch does not introduce any of the things you mention, all of those things were already there. I agree that they look odd, and I can change them to be less odd in a separate change, but I wouldn't want to mix that in with the change at hand. Do you see any problem with the parts of the code that are actually changed by this patch?

IOMMU support exists for amd64, but it is not enabled by default, and no bounce buffer memory is allocated by mpr attach with IOMMU support disabled. and mpr works fine without any IOMMU.

replace the old fix with all new better fix

The question I have is why any bounce buffer memory is being allocated at all by the creation of this tag. The code that sets up the template for creating buffer_dmat does not specify any constraints that would need bounce-buffering. Only low/highaddr and alignment can cause bounce-buffering to be needed, and the template does not set any of those. So the constraint that leads to the allocation of bounce-buffer memory must be inherited from some parent device. The only one I see that looks possibly relevant for your system is the one in opalpci_attach(), but that is supposedly only for POWER8 systems and your Blackbird system is supposed to be a POWER9 system. So I think it would help to find where the constraint that is triggering bounce-buffering is coming from.

Code change looks fine, commit comment has a typo (certian vs. certain).

The code change is fine, but the commit comment has several typos (appropraite vs. appropriate, phyiscal vs. physical, 2096 vs. 4096).

this is fine as far as it goes, but the other places that already do "mpi3mr_atomic_dec(&tgtdev->block_io)" should also be converted to use the new mpi3mr_atomic_dec_if_not_zero().

In D49749#1136554, @chandrakanth.patil_broadcom.com wrote:

In D49749#1135942, @chs wrote:

Ok, I didn't realize that "task management" always meant "target reset" here. Not waiting for in-progress commands makes sense in this context. Could you add a comment to the code explaining this?

Another thing that I noticed while looking at this patch was that all the other places that decrement block_io do it in this style:

if (mpi3mr_atomic_read(&tgtdev->block_io) > 0)
        mpi3mr_atomic_dec(&tgtdev->block_io);

[ckp] Agreed, we need to check the block_io > 0 before decrementing block_io, otherwise if other thread has already decremented then the counter may go negative. I will add new patch for that. Thank you.

whereas this one you've added does not check the current value before decrementing.  is there some reason why this new instance doesn't follow the pattern of the others?

note that the pattern of the others also seems wrong... the check of the current value is not atomic with the decrement, so it looks like one thread could check the value and see that it is not zero, another thread could execute one of the paths that sets the current value to zero, and then the first thread could proceed to decrement the value and cause the value to become -1.  is there something that prevents this scenario from occurring?

[ckp] I didnt get how two thread can access the atomic variable? Is this because the v (variable) is non atomic? but atomic_subtract_int() api should take care of atomicity right? if not then please let us know any other way of making the variable and operation atomic. do we need to use this atomic_fetchadd_int() API to achieve atomicity?

#define mpi3mr_atomic_dec(v) atomic_subtract_int(&(v)->val, 1)

Ok, I didn't realize that "task management" always meant "target reset" here. Not waiting for in-progress commands makes sense in this context. Could you add a comment to the code explaining this?

This change blocks new I/Os from being submitted while a task management command is in progress, but wouldn't you need to also wait for any in-progress I/Os to complete before submitting the task management command? I think you would need to wait for those if your goal is to avoid having the firmware processing I/O commands and task management commands together (which is what this change appears to be trying to do).

I tried building with the new patch and found more problems:

in addition the changes you have here already, you'll also need to add the same line that you added to sys/conf/NOTES to sys/amd64/conf/GENERIC and sys/i386/conf/GENERIC.

fix a bug I introduced when changing the calculation of cpi->maxio.

fixed the typo in the subject and review title

I was going to do this this morning but you beat me to it. sorry for the noise.

removed unnecessary roundup() and fixed some PAGE_SIZE vs MPI3MR_4K_PGSZ confusion.

This patch had also tried to resolve some confusion between the OS PAGE_SIZE and the driver's MPI3MR_4K_PGSZ, but I got some cases wrong. I'll fix those in the next version of the patch as well.

There are some console messages printed by the early loading code, but these happen before the regular kernel printf code is initialized, so you can only see them if you enable "options EARLY_PRINTF" in your kernel config and also use "options UART_NS8250_EARLY_PORT=0x3f8" to set which serial port the early printf output should go to. (eg. the 0x3f8 in this example is com1). you also need to boot with "-v" to enable "bootverbose" messages for the ucode early load activity to be printed.

Updated diff for latest review comments.

move the code copied from cpucontrol to a new file so that we can share it between the kernel and cpucontrol.

I added the handling you requested for the loader blob containing all of the amd ucode files concatenated together.

updated patch with requested changes

I agree, this msgbuftrigger thing is rather loosey-goosey. I'm just trying to have all the msgbuf paths achieve a consistent level of loosey-gooseyness.